New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OSR fixes - devtools, dialogs #10510

Merged
merged 17 commits into from Nov 13, 2017

Conversation

Projects
None yet
7 participants
@brenca
Member

brenca commented Sep 13, 2017

This PR aims to fix the following bugs:

  • devtools on Windows would crash when the user typed anything in the console (in detached state). This was related to the datalist element support that I added a while back. The normal window with the devtools of an offscreen one, trying to show suggestions (this is where the datalist comes into play) confused the system enough to crash.
  • devtools for an offscreen window wouldn't render properly on MacOS
  • any dialogs (alert, confirm, download) would show the hidden dummy window on MacOS because these were modal dialogs. I fixed this by forcing the dialogs to be detached from any window in case the parent is offscreen.
  • datalist elements on windows wouldn't properly set the input's value when a suggestion was accepted. ( #9860 )
  • datalist elements didn't render at the proper position ( #11035 )

@zeke zeke requested a review from zcbenz Sep 26, 2017

@@ -226,12 +226,13 @@ void AutofillPopupView::OnPaint(gfx::Canvas* canvas) {
SkBitmap bitmap;
#if defined(ENABLE_OSR)
cc::SkiaPaintCanvas* paint_canvas = nullptr;

This comment has been minimized.

@zcbenz

zcbenz Oct 3, 2017

Contributor

Can you wrap it in a std::unique_ptr? We should try avoiding managing memory manually.

autofill_popup_->CreateView(
frame_host,
wc->IsOffScreenOrEmbedderOffscreen(),
web_contents->IsOffScreenOrEmbedderOffscreen(),

This comment has been minimized.

@zcbenz

zcbenz Oct 3, 2017

Contributor

For better design, we should avoid interacting with atom::api::WebContents here by reading the offscreen property from the WebContentsPreferences.

The embedder WebContents should also inherit the offscreen property when getting created, the existence of IsOffScreenOrEmbedderOffscreen is not good design.

@@ -112,7 +112,8 @@ void SetupDialogForProperties(NSOpenPanel* dialog, int properties) {
// Run modal dialog with parent window and return user's choice.
int RunModalDialog(NSSavePanel* dialog, atom::NativeWindow* parent_window) {
__block int chosen = NSFileHandlingPanelCancelButton;
if (!parent_window || !parent_window->GetNativeWindow()) {
if (!parent_window || !parent_window->GetNativeWindow() ||
parent_window->IsOffScreenDummy()) {

This comment has been minimized.

@zcbenz

zcbenz Oct 3, 2017

Contributor

The dialog API should not need to know whether a window is offscreen, this is the responsibility of the caller.

You can pass the offscreen state to CommonWebContentsDelegate and WebDialogHelper and let those classes decide how to show dialogs.

@ckerr

This comment has been minimized.

Member

ckerr commented Oct 23, 2017

brenca, do you have time to update the patch wrt the review comments?

@brenca

This comment has been minimized.

Member

brenca commented Nov 2, 2017

Yes, gonna do the upgrade today and tomorrow

@brenca brenca requested review from electron/docs as code owners Nov 2, 2017

@brenca

This comment has been minimized.

Member

brenca commented Nov 3, 2017

@zcbenz I've addressed your comments, thanks for the review, and sorry for the slow response. I had to keep the embedder offscreen check for osr webview tags, those are handled internally by chromium as not-offscreen, yet the generated popups must be offscreen to display properly. I rewrote that part of the code however to be more clean, and to use the web_preferences.

@brenca

This comment has been minimized.

Member

brenca commented Nov 3, 2017

@zcbenz ready for review.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Nov 13, 2017

I'm doing a rebase to fix line endings and some unexpected checked submodules.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Nov 13, 2017

@brenca I'm reverting the change to the did-change-theme-color event, it is an API breakage and we should discuss it before making the change.

@zcbenz

zcbenz approved these changes Nov 13, 2017

The failing test is circle ci's fault, I'm going to merge with it ignored.

@zcbenz zcbenz merged commit 4d364fa into electron:master Nov 13, 2017

4 of 6 checks passed

ci/circleci: electron-linux-x64 Your tests failed on CircleCI
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Nov 13, 2017

@alexeykuzmin The changes here are very likely to have merge conflicts with chrome61 branch, let me know if you need help.

@alexeykuzmin

This comment has been minimized.

Contributor

alexeykuzmin commented Nov 13, 2017

@zcbenz Yeah, there're merge conflicts now in the "atom/browser/osr/osr_output_device.cc", but they seem to be pretty trivial. Thanks!

@MichaelF1

This comment has been minimized.

MichaelF1 commented Nov 16, 2017

I am sorry for the question, is there a fix for this problem and how can I fix it? (Newbie with electron)

Regards

@StillOnMyWay

This comment has been minimized.

StillOnMyWay commented Nov 30, 2017

@brenca Regarding the datalist update, when can we expect to see that fix go live?

@brenca

This comment has been minimized.

Member

brenca commented Nov 30, 2017

@StillOnMyWay Sure, this fix was pulled in some time ago, it should be in the next released version.

@idobh2

This comment has been minimized.

idobh2 commented Dec 7, 2017

@brenca I see that this PR is included in 1.8.2-beta3, but I still see the issue, for both detached or in-window devtools console.
I'm on windows 10 1709. I'll try to have a look and update if I have something helpful to say.

@brenca

This comment has been minimized.

Member

brenca commented Dec 7, 2017

@idobh2 Which issue? If you are talking about the offscreen window's devtools crash, that fix was kinda lost when @zcbenz merged this PR, I've posted another one with a fix for that a while ago.

@idobh2

This comment has been minimized.

idobh2 commented Dec 7, 2017

@brenca Yes, this is the one. Sorry, I've looked for an issue or PR and somehow missed it. Thanks!

@brenca

This comment has been minimized.

Member

brenca commented Dec 7, 2017

It's a bit hard to find, so actually I'm sorry. In the meantime, someone else posted an issue about this #11375.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment