Fix #8911 #9292
Fix #8911 #9292
Conversation
There was a problem hiding this comment.
LGTM on the approach, requires some minor fixes. Thanks @22222 !
| @@ -861,6 +861,12 @@ void WebContents::Observe(int type, | |||
| } | |||
| } | |||
|
|
|||
| void WebContents::BeforeUnloadDialogCancelled() { | |||
| if (deferred_load_url_.id) { | |||
There was a problem hiding this comment.
BeforeUnloadDialogCancelled will also be called for any cancellation inside iframes, we do not want to clear the defer load params in that situation, it would have been better if there was an argument that contained the frame id. The only way now is to check if there is an pending entry or not. This observer will be called only after clearing navigation controller states for this webContents, so we can be sure any pending entry would have been cleared.
if (deferrer_load_url_.id && !controller.GetPendingEntry())
There was a problem hiding this comment.
Awesome, thanks for this feedback. I figured there'd be some special case I hadn't considered.
spec/api-browser-window-spec.js
Outdated
| it('fires for each close attempt', function (done) { | ||
| var closeCount = 5 | ||
|
|
||
| var timeoutHandle = setTimeout(() => { |
There was a problem hiding this comment.
The timeoutHandle can lead to a flaky spec, better to remove it. Sometimes the spec may take longer than expected. We do have an overall timeout.
There was a problem hiding this comment.
Good point, I should have rethought that before I submitted the pull request. That was all just to workaround issues with the after test hooks not being able to close a page that has a beforeunload handler stopping that.
I'll rework these tests so they're more like the other beforeunload tests (where the beforeunload events stop preventing the window from closing after a fixed number of calls).
spec/api-browser-window-spec.js
Outdated
| var closeCount = 5 | ||
|
|
||
| var timeoutHandle = setTimeout(() => { | ||
| w.webContents.executeJavaScript('isDone = true') |
There was a problem hiding this comment.
executeJavaScript is an async operation, spec should be ended once its resolved.
22222
force-pushed
the
issue8911
branch
2 times, most recently
from
cd80827 to
6c643a4
Compare
| void WebContents::BeforeUnloadDialogCancelled() { | ||
| if (deferred_load_url_.id) { | ||
| auto web_contents = managed_web_contents()->GetWebContents(); | ||
| auto& controller = web_contents->GetController(); |
There was a problem hiding this comment.
This can be just auto& controller = web_contents()->GetController();
|
Thanks for this @22222 👍 👍 |
This would be an addition to the changes in #8724 to handle cases where the navigation is cancelled.
I'm not completely sure that overriding the
BeforeUnloadDialogCancelledmethod is the best way to handle this, or if there's more that should be cleaned up. This fixes the one case I was testing for (#8911) and doesn't break any unit tests (at least when I run them), but I didn't do much other testing to see if there are any other negative side-effects.And this is my first time even looking at the Electron source code, so don't make any assumptions that I know what I'm doing here.
Fixes #8911