Inform the WebFrameClient of BeforeUnload/Unload handlers

Inform the WebFrameClient of BeforeUnload/Unload handlers

This CL makes it possible for a WebFrameClient to be made aware of the registration and unregistration of BeforeUnload handlers. This is the first step in refactoring the sudden termination API, with the following chromium-side patch https://chromiumcodereview.appspot.com/857213003/.

BUG=365039
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188805

[clamy, 2015-01-20 16:45:20]

clamy@chromium.org changed reviewers:
+ creis@chromium.org, japhet@chromium.org, nasko@chromium.org

[clamy, 2015-01-20 16:47:02]

@creis, nasko, japhet: PTAL. This is the first patch to clean up the sudden
termination logic and skip executing BeforeUnload in cross-site navigations when
no handler is registered in the Frame (see
https://docs.google.com/a/chromium.org/document/d/1iivJbM4GBoJmJbTkG7LP9DPTsL...
for more info).

https://chromiumcodereview.appspot.com/861773003/diff/1/Source/core/frame/Loc...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://chromiumcodereview.appspot.com/861773003/diff/1/Source/core/frame/Loc...
Source/core/frame/LocalDOMWindow.cpp:232: disableSuddenTermination();
Note: this call as well as the equivalent ones down below will be removed in a
following patch when the support for the per-frame sudden termination tracking
has landed in Chromium.

[Nate Chapin, 2015-01-20 18:46:49]

Nit: BeofreUnload->BeforeUnload in change description.

https://codereview.chromium.org/861773003/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/861773003/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:246: enableSuddenTermination();
Will we eventually be able to remove these enable/disable calls?

https://codereview.chromium.org/861773003/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:247:
domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
Is this safe? domWindow->frame() can be null at very annoying times.

https://codereview.chromium.org/861773003/diff/20001/Source/core/loader/Frame...
File Source/core/loader/FrameLoaderClient.h (right):

https://codereview.chromium.org/861773003/diff/20001/Source/core/loader/Frame...
Source/core/loader/FrameLoaderClient.h:235: enum SuddenTerminationDisablerType {
This enum and the matching one in WebFrameClient.h should be added to
Source/web/AssertMatchingEnums.cpp

[clamy, 2015-01-21 13:46:32]

Thanks!

https://chromiumcodereview.appspot.com/861773003/diff/20001/Source/core/frame...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://chromiumcodereview.appspot.com/861773003/diff/20001/Source/core/frame...
Source/core/frame/LocalDOMWindow.cpp:246: enableSuddenTermination();
On 2015/01/20 18:46:49, Nate Chapin wrote:
> Will we eventually be able to remove these enable/disable calls?

Yes, when the chromium side patch lands we should remove those. However we
cannot get completely rid of blink::Platform::suddenTerminationChanged, because
we cannot track per-Frame the disables of sudden termination caused by pending
DOM local storage callbacks. It seems it has to be done per-process.

https://chromiumcodereview.appspot.com/861773003/diff/20001/Source/core/frame...
Source/core/frame/LocalDOMWindow.cpp:247:
domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
On 2015/01/20 18:46:49, Nate Chapin wrote:
> Is this safe? domWindow->frame() can be null at very annoying times.

In that case I added a check for domWindow->frame() before calling
suddenTerminationDisablerChanged. It did not seem to cause bot failures without
it though.

https://chromiumcodereview.appspot.com/861773003/diff/20001/Source/core/loade...
File Source/core/loader/FrameLoaderClient.h (right):

https://chromiumcodereview.appspot.com/861773003/diff/20001/Source/core/loade...
Source/core/loader/FrameLoaderClient.h:235: enum SuddenTerminationDisablerType {
On 2015/01/20 18:46:49, Nate Chapin wrote:
> This enum and the matching one in WebFrameClient.h should be added to
> Source/web/AssertMatchingEnums.cpp

Done.

[Nate Chapin, 2015-01-21 19:51:42]

LGTM

https://codereview.chromium.org/861773003/diff/20001/Source/core/frame/LocalD...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/861773003/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:246: enableSuddenTermination();
On 2015/01/21 13:46:31, clamy wrote:
> On 2015/01/20 18:46:49, Nate Chapin wrote:
> > Will we eventually be able to remove these enable/disable calls?
> 
> Yes, when the chromium side patch lands we should remove those. However we
> cannot get completely rid of blink::Platform::suddenTerminationChanged,
because
> we cannot track per-Frame the disables of sudden termination caused by pending
> DOM local storage callbacks. It seems it has to be done per-process.

Interesting, I would have thought that local storage could be per-frame as well,
but I know nothing about the chromium-side implementation.

https://codereview.chromium.org/861773003/diff/20001/Source/core/frame/LocalD...
Source/core/frame/LocalDOMWindow.cpp:247:
domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
On 2015/01/21 13:46:31, clamy wrote:
> On 2015/01/20 18:46:49, Nate Chapin wrote:
> > Is this safe? domWindow->frame() can be null at very annoying times.
> 
> In that case I added a check for domWindow->frame() before calling
> suddenTerminationDisablerChanged. It did not seem to cause bot failures
without
> it though.

I did a quick look and didn't see anything that would prevent an attempt to add
an event listener to a detached window, but it may be difficult to get into that
situation in practice. I would not be at all surprised if we don't have test
coverage for it.

If you are willing to tolerate the risk of a fire drill, you could omit the
null-checks and be prepared to deal with a potential crash report in a couple
days. I'd lean toward including the null-checks just to be safe, though.

[Charlie Reis, 2015-01-21 23:09:25]

Rubber stamp LGTM (as non-owner).  That's unfortunate about DOM storage
callbacks being per-process, but we can deal with it.

https://codereview.chromium.org/861773003/diff/40001/public/platform/Platform.h
File public/platform/Platform.h (right):

https://codereview.chromium.org/861773003/diff/40001/public/platform/Platform...
public/platform/Platform.h:401: // is preferable to disable sudden on a
per-frame level via
nit: sudden termination

[clamy, 2015-01-22 10:34:28]

Thanks!

https://chromiumcodereview.appspot.com/861773003/diff/40001/public/platform/P...
File public/platform/Platform.h (right):

https://chromiumcodereview.appspot.com/861773003/diff/40001/public/platform/P...
public/platform/Platform.h:401: // is preferable to disable sudden on a
per-frame level via
On 2015/01/21 23:09:25, Charlie Reis wrote:
> nit: sudden termination

Done.

[clamy, 2015-01-22 10:34:39]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-01-22 10:35:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/861773003/60001

[commit-bot: I haz the power, 2015-01-22 11:45:02]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188805