Keep track in the browser of which frames have onunload and onbeforeunload handlers.

Keep track in the browser of which frames have onunload and onbeforeunload handlers.

This allows PlzNavigate to only pause fetching the request if it knows that the page has an onbeforeunload handler, where today it currently always goes to the renderer. This avoids delaying the network requests in the 95% cases that don't onbeforeunload handler on a process hop to the renderer.

BUG=365039, 705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=dcheng@chromium.org, nasko@chromium.org

Review-Url: https://codereview.chromium.org/2783723002 .
Cr-Commit-Position: refs/heads/master@{#460581}
Committed: https://chromium.googlesource.com/chromium/src/+/a2c84a7664eceb5cbeaae5bf1ec32da0060e3097

[jam, 2017-03-28 17:19:36]

Description was changed from

==========
Reland of Camille's change to plumb sudden termination per frame.

Original cl: https://codereview.chromium.org/857213003/

BUG=365039,705559
==========

to

==========
Reland of Camille's change to plumb sudden termination per frame.

Original cl: https://codereview.chromium.org/857213003/

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-28 17:19:51]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 17:20:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-28 18:11:37]

Description was changed from

==========
Reland of Camille's change to plumb sudden termination per frame.

Original cl: https://codereview.chromium.org/857213003/

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep track of in the browser of which frames have onunload and onbeforeunload
handlers.

This is a subset of Camille's change https://codereview.chromium.org/857213003/.



BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-28 18:13:19]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:13:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-28 18:42:57]

Description was changed from

==========
Keep track of in the browser of which frames have onunload and onbeforeunload
handlers.

This is a subset of Camille's change https://codereview.chromium.org/857213003/.



BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep track of in the browser of which frames have onunload and onbeforeunload
handlers.

This is a subset of Camille's change https://codereview.chromium.org/857213003/.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-28 18:43:29]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:44:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-28 18:46:04]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 18:46:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 19:19:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 19:19:10]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jam, 2017-03-28 22:00:45]

Description was changed from

==========
Keep track of in the browser of which frames have onunload and onbeforeunload
handlers.

This is a subset of Camille's change https://codereview.chromium.org/857213003/.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep track in the browser of which frames have onunload and onbeforeunload
handlers.

This is a subset of Camille's change https://codereview.chromium.org/857213003/.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-28 22:01:19]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 22:01:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-28 22:14:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 22:14:59]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[jam, 2017-03-28 23:03:57]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-28 23:04:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-28 23:07:15]

Patchset #5 (id:70001) has been deleted

[commit-bot: I haz the power, 2017-03-28 23:59:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-28 23:59:48]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2017-03-29 01:51:22]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 01:52:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 02:23:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 02:23:29]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[jam, 2017-03-29 14:46:56]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 14:47:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-29 15:51:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 15:51:21]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[jam, 2017-03-29 18:23:52]

Description was changed from

==========
Keep track in the browser of which frames have onunload and onbeforeunload
handlers.

This is a subset of Camille's change https://codereview.chromium.org/857213003/.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep track in the browser of which frames have onunload and onbeforeunload
handlers.

This allows PlzNavigate to only pause fetching the request if it knows that the
page has an onbeforeunload handler, where today it currently always goes to the
renderer.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-29 18:23:53]

jam@chromium.org changed reviewers:
+ nasko@chromium.org

[jam, 2017-03-29 18:37:46]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 18:38:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-29 19:08:57]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-03-29 20:14:38]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[jam, 2017-03-29 20:15:27]

jam@chromium.org changed reviewers:
+ dcheng@chromium.org

[commit-bot: I haz the power, 2017-03-29 20:15:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-29 20:15:28]

Daniel: webkit
Nasko: the rest

[jam, 2017-03-29 21:10:21]

Description was changed from

==========
Keep track in the browser of which frames have onunload and onbeforeunload
handlers.

This allows PlzNavigate to only pause fetching the request if it knows that the
page has an onbeforeunload handler, where today it currently always goes to the
renderer.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep track in the browser of which frames have onunload and onbeforeunload
handlers.

This allows PlzNavigate to only pause fetching the request if it knows that the
page has an onbeforeunload handler, where today it currently always goes to the
renderer. This avoids delaying the network requests in the 95% cases that don't
onbeforeunload handler on a process hop to the renderer.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[jam, 2017-03-29 21:20:51]

The CQ bit was unchecked by jam@chromium.org

[jam, 2017-03-29 21:30:47]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 21:31:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[nasko, 2017-03-29 21:45:17]

For navigations, the only event that matters is the beforeunload one. It might
be useful to keep this CL to a minimum and exclude the unload event code, as it
can affect sudden termination status of a process, but not navigation.

https://codereview.chromium.org/2783723002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2783723002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2728: bool
RenderFrameHostImpl::ShouldDispatchUnload() {
This actually doesn't make sense to have. On cross-process navigations, the act
of swapping out the old RenderFrameHost will run the unload handler, if one is
present. In same-process navigations, it will be done entirely internally to
Blink, so content/ doesn't have to care at all. Certainly the browser side
doesn't need to know about this.

https://codereview.chromium.org/2783723002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2783723002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1427:
GetMainFrame()->ShouldDispatchUnload());
Why would unload handler matter for NeedToFireBeforeUnload?

[commit-bot: I haz the power, 2017-03-29 22:12:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-29 22:12:17]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[jam, 2017-03-29 22:15:43]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 22:16:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-03-29 22:19:09]

https://codereview.chromium.org/2783723002/diff/160001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2783723002/diff/160001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1427:
GetMainFrame()->ShouldDispatchUnload());
On 2017/03/29 21:45:17, nasko wrote:
> Why would unload handler matter for NeedToFireBeforeUnload?

That was my initial thought as well. I had to add the unload case to fix a few
tests (see patchset 6 -> 7. This fixed 
ExtensionApiTest.MessagingOnUnload
FastUnloadTest.UnloadHidden
PrerenderBrowserTest.PrerenderUnload 

It was because of how the UnloadController works today. It turns out that this
method's comments also specifically mention beforeunload and unload:
https://cs.chromium.org/chromium/src/content/public/browser/web_contents.h?rc...
"Returns true if the before unload and unload listeners need to fired."

[dcheng, 2017-03-29 23:03:46]

Blink LGTM

I don't feel super strongly about keeping the comments as-is, but it does read
confusingly (since it's not obvious that the beforeunload / unload things are
just bumping counts)

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1440: // process.
Nit: please keep this comment.

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1449:
removeBeforeUnloadEventListener(this);
This would ideally be commented too.

[nasko, 2017-03-29 23:31:26]

TIL WebContents::NeedToFireBeforeUnload doesn't do what it is named to do :(.

https://codereview.chromium.org/2783723002/diff/180001/content/browser/browse...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/browse...
content/browser/browser_side_navigation_browsertest.cc:286:
"data:text/html,<html><script>window.onbeforeunload=function(e)"
Why not create a simple html file in content/test/data and use it here and the
WebContents browser tests? It seems that such a file will be beneficial in the
future too, as meacer@ is adding more tests that can use it.

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:680:
weak_factory_.GetWeakPtr(), false, error_code));
Let's try to reason through this one, as it isn't obvious by looking at it. 
If we post a task to call the OnRequestFailed the NavigationRequest remains
valid and associated with the FrameTreeNode. It is possible that the event loop
already has a task to perform a renderer-initiated navigation queued up before
the task is posted. When it is executed, it will be ignored, as the
browser-initiated navigation won't be cancelled (checks inside
NavigatorImpl::OnBeginNavigation). Also an IPC will be sent to the renderer to
stop the navigation.

If the navigation has user gesture, then it will cancel the existing
NavigationRequest, but then the task won't execute because of the WeakPtr will
be invalidated.

Prior to this CL, we would send the IPC to execute any beforeunload handlers and
then finish with the task. If we had the same case as above, I guess we are in
the same situation and the same events will happen.

Seems fine :)

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (left):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:317:
subframe_rfh->SendBeforeUnloadACK(true);
Aren't we losing coverage for a beforeunload handler present, which allows
navigation to proceed?

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:436: bool
ShouldDispatchUnload();
Since there is no dispatching of unload handler happening from the browser side,
let's call that HasUnloadHandler or something similar that indicates that
subframes are consulted too.

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (left):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1267:
EXPECT_TRUE(rfh2->is_waiting_for_beforeunload_ack());
Why not set_has_beforeunload_handlers on rfh2? We already fudge some of the
state on rfh2 above anyway.

https://codereview.chromium.org/2783723002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1427:
GetMainFrame()->ShouldDispatchUnload());
This seems very very broken. Let's put a TODO to clean this up as it is
incorrect/inconsistent. Feel free to have it TODO(nasko) ;).

https://codereview.chromium.org/2783723002/diff/180001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2783723002/diff/180001/content/common/frame_m...
content/common/frame_messages.h:1618: // Messages to signal the presence or
absence of BeforeUnload or Unload handlers
nit: You use capitalization inconsistently for the name of the two events,
lowercase is the web platform version, so might as well stick with that.

https://codereview.chromium.org/2783723002/diff/180001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2783723002/diff/180001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4769: default:
Omitting the default: clause should be fine and will allow the compiler to
enforce that all possible enum values are checked in the switch statement.

[nasko, 2017-03-29 23:42:08]

LGTM on content, assuming the comments will be addressed in an immediate follow
up CL, so this one can have a chance of landing for the next canary cut.

[jam, 2017-03-29 23:48:06]

Description was changed from

==========
Keep track in the browser of which frames have onunload and onbeforeunload
handlers.

This allows PlzNavigate to only pause fetching the request if it knows that the
page has an onbeforeunload handler, where today it currently always goes to the
renderer. This avoids delaying the network requests in the 95% cases that don't
onbeforeunload handler on a process hop to the renderer.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep track in the browser of which frames have onunload and onbeforeunload
handlers.

This allows PlzNavigate to only pause fetching the request if it knows that the
page has an onbeforeunload handler, where today it currently always goes to the
renderer. This avoids delaying the network requests in the 95% cases that don't
onbeforeunload handler on a process hop to the renderer.

BUG=365039,705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
R=dcheng@chromium.org, nasko@chromium.org

Review-Url: https://codereview.chromium.org/2783723002 .
Cr-Commit-Position: refs/heads/master@{#460581}
Committed:
https://chromium.googlesource.com/chromium/src/+/a2c84a7664eceb5cbeaae5bf1ec3...
==========

[jam, 2017-03-29 23:48:08]

Committed patchset #10 (id:180001) manually as
a2c84a7664eceb5cbeaae5bf1ec32da0060e3097 (presubmit successful).

[jam, 2017-03-30 14:53:23]

thanks all, the followup comments are in:
https://codereview.chromium.org/2781383002

https://codereview.chromium.org/2783723002/diff/180001/content/browser/browse...
File content/browser/browser_side_navigation_browsertest.cc (right):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/browse...
content/browser/browser_side_navigation_browsertest.cc:286:
"data:text/html,<html><script>window.onbeforeunload=function(e)"
On 2017/03/29 23:31:26, nasko wrote:
> Why not create a simple html file in content/test/data and use it here and the
> WebContents browser tests? It seems that such a file will be beneficial in the
> future too, as meacer@ is adding more tests that can use it.

Done.

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/navigator_impl_unittest.cc (left):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
content/browser/frame_host/navigator_impl_unittest.cc:317:
subframe_rfh->SendBeforeUnloadACK(true);
On 2017/03/29 23:31:26, nasko wrote:
> Aren't we losing coverage for a beforeunload handler present, which allows
> navigation to proceed? 

we already have test cases for this, i.e. all the browser tests that do have
beforeunload handlers. IMO all these unit tests aren't really testing this,
since it's all mocked up and using different code paths than production. they
just had to do this to not hang. since the unit tests should be about testing
one thing, and none of these tests are about onbeforeunload, IMO this is an
improvement :)

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:436: bool
ShouldDispatchUnload();
On 2017/03/29 23:31:26, nasko wrote:
> Since there is no dispatching of unload handler happening from the browser
side,
> let's call that HasUnloadHandler or something similar that indicates that
> subframes are consulted too. 

Done.

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager_unittest.cc (left):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager_unittest.cc:1267:
EXPECT_TRUE(rfh2->is_waiting_for_beforeunload_ack());
On 2017/03/29 23:31:26, nasko wrote:
> Why not set_has_beforeunload_handlers on rfh2? We already fudge some of the
> state on rfh2 above anyway. 

similar to my other reply, this isn't what the unit test is testing.

https://codereview.chromium.org/2783723002/diff/180001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/2783723002/diff/180001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1427:
GetMainFrame()->ShouldDispatchUnload());
On 2017/03/29 23:31:26, nasko wrote:
> This seems very very broken. Let's put a TODO to clean this up as it is
> incorrect/inconsistent. Feel free to have it TODO(nasko) ;).

Done.

https://codereview.chromium.org/2783723002/diff/180001/content/common/frame_m...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/2783723002/diff/180001/content/common/frame_m...
content/common/frame_messages.h:1618: // Messages to signal the presence or
absence of BeforeUnload or Unload handlers
On 2017/03/29 23:31:26, nasko wrote:
> nit: You use capitalization inconsistently for the name of the two events,
> lowercase is the web platform version, so might as well stick with that.

Done.

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (left):

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1440: // process.
On 2017/03/29 23:03:46, dcheng wrote:
> Nit: please keep this comment.

Done.

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/2783723002/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp:1449:
removeBeforeUnloadEventListener(this);
On 2017/03/29 23:03:46, dcheng wrote:
> This would ideally be commented too.

all 4 would need comment, to be consistent, or perhaps even better the method
name would be clearer. but since this existed before, I think it's orthogonal.

[achuithb, 2017-04-05 17:40:43]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2803633004/ by achuith@chromium.org.

The reason for reverting is: Creating a revert patchset for testing. Will not be
submitted..

[jam, 2017-04-10 22:11:13]

A revert of this CL (patchset #10 id:180001) has been created in
https://codereview.chromium.org/2812743002/ by jam@chromium.org.

The reason for reverting is: Exposed races with FrameHostMsg_DidStopLoading

BUG=710062
.