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

Add 'will-prevent-unload' event for #2579. #9331

Merged
merged 2 commits into from May 11, 2017

Conversation

Projects
None yet
2 participants
@22222
Copy link
Contributor

22222 commented Apr 30, 2017

This change is based on my comment in issue 2579. I have very little experience with the Electron code, so I hope that "help/beginner" tag still applies.

Restructure

The biggest part of this change is getting access to the event emitting methods of the atom::api::WebContents class from the atom::AtomJavaScriptDialogManager class. That requirement led me to shuffling around a couple dependencies.

I based the changes to atom::AtomJavaScriptDialogManager on the existing atom::BluetoothChooser class: have a constructor that accepts an api::WebContents* object, save a reference to it, and use its Emit method later.

The second part of that change is in constructing the atom::AtomJavaScriptDialogManager object. Right now creating and managing that dialog_manager object happens in the atom::CommonWebContentsDelegate base class, which isn't ideal for this change since it doesn't have access to any of the Emit methods. So I moved that (including all references to atom::AtomJavaScriptDialogManager) into the atom::api::WebContents class.

One thing that makes me a little nervous about that change is this comment from common_web_contents_delegate.h:

  // The stored InspectableWebContents object.
  // Notice that web_contents_ must be placed after dialog_manager_, so we can
  // make sure web_contents_ is destroyed before dialog_manager_, otherwise a
  // crash would happen.
  std::unique_ptr<brightray::InspectableWebContents> web_contents_;

The dialog_manager_ field is in a subclass now, so it should still be destroyed before web_contents_ field. But this change does affect the destruction order, so if there are any other dependencies on the destruction order then it's possible that some other problem could come from this.

Reloads

One additional piece of information that the RunBeforeUnloadDialog method has is an is_reload flag. The Chromium browser uses that to choose a different title for the confirmation dialog if the unload is for a reload (Do you want to reload this site? instead of Do you want to leave this site?).

Originally I planned to make that is_reload flag available in the new will-prevent-unload event. But in my testing the flag was always false, even for a reload (using the reload menu option from the default app). So at least for now, I left the is_reload flag out since it would be misleading to provide it if it doesn't work, and it would probably be possible to add later if there's a demand for it.

@kevinsawicki kevinsawicki self-assigned this May 9, 2017

@kevinsawicki
Copy link
Contributor

kevinsawicki left a comment

This looks good, left one minor comment, also could you resolve the conflicts in this branch? Thanks.

#include "atom/browser/native_window.h"
#include "atom/browser/ui/message_box.h"
#include "base/bind.h"
#include "base/strings/utf_string_conversions.h"
#include "ui/base/l10n/l10n_util.h"

This comment has been minimized.

@kevinsawicki

kevinsawicki May 9, 2017

Contributor

I don't think this include is needed.

@22222 22222 force-pushed the 22222:issue2579 branch from 000d834 to f6c6cc2 May 9, 2017

@22222

This comment has been minimized.

Copy link
Contributor Author

22222 commented May 9, 2017

Thanks for reviewing this.

I made the requested change and resolved the conflict. Let me know if you need any other changes.

@22222 22222 force-pushed the 22222:issue2579 branch from f6c6cc2 to 38637cc May 11, 2017

@22222 22222 force-pushed the 22222:issue2579 branch from 38637cc to 4044548 May 11, 2017

Minor updates to example
* const-ing
* mainWindow -> win
* add dialog require
@kevinsawicki

This comment has been minimized.

Copy link
Contributor

kevinsawicki commented May 11, 2017

Thanks so much for this 👍 🚢

@kevinsawicki kevinsawicki merged commit 190c9c9 into electron:master May 11, 2017

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment