Skip BeforeUnload on cross-site navigations when there are no handlers

Skip BeforeUnload on cross-site navigations when there are no handlers

This CL removes the IPC roundtrip where the browser waits for the renderer to
execute the BeforeUnload event when there are no beforeUnload handlers
registered for the frame.

BUG=365039

[clamy, 2015-02-16 17:36:29]

clamy@chromium.org changed reviewers:
+ carlosk@chromium.org, creis@chromium.org

[clamy, 2015-02-16 17:43:53]

@creis: PTAL
@carlosk: FYI

This is the CL to skip waiting beforeUnload when there are no handlers. It is
creating some flakiness on the WebNavigation API browser tests, which I cannot
reproduce locally. I am not quite sure what to do there: maybe not skip
beforeUnload when dealing with extensions?

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1565: // beforeUnload
handlers.
We currently only skip waiting for beforeUnload in navigations because skipping
it on close require a lot of rewrite of the UnloadController and the
FastUnloadController in chrome which I did not want to do in that patch.

[carlosk, 2015-02-17 12:52:09]

Cool with just one minor suggestion.

https://codereview.chromium.org/903043002/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/903043002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1006: 
Suggestion: split it here into a new test.

[Charlie Reis, 2015-02-17 23:04:31]

LGTM with nits.

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (left):

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:275:
main_test_rfh()->SendBeforeUnloadACK(true);
We should include a PlzNavigate test for the case that a beforeunload handler is
present as well.

https://codereview.chromium.org/903043002/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/903043002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1007: // Now navigate
to a new site. There is no beforeUnload handlers, so we should
nit: There are

[clamy, 2015-02-18 13:27:02]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[clamy, 2015-02-18 13:27:23]

Thanks!

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (left):

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:275:
main_test_rfh()->SendBeforeUnloadACK(true);
On 2015/02/17 23:04:31, Charlie Reis wrote:
> We should include a PlzNavigate test for the case that a beforeunload handler
is
> present as well.

I added a test to check that we properly wait before starting the navigation if
a handler is present as well.

https://codereview.chromium.org/903043002/diff/100001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_unittest.cc (right):

https://codereview.chromium.org/903043002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1006: 
On 2015/02/17 12:52:09, carlosk wrote:
> Suggestion: split it here into a new test.

I think it makes sense for it to be in this test, which is testing beforeUnload
in general.

https://codereview.chromium.org/903043002/diff/100001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_unittest.cc:1007: // Now navigate
to a new site. There is no beforeUnload handlers, so we should
On 2015/02/17 23:04:31, Charlie Reis wrote:
> nit: There are

Done.

[clamy, 2015-02-18 13:27:41]

The CQ bit was checked by clamy@chromium.org

[commit-bot: I haz the power, 2015-02-18 13:28:27]

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

[commit-bot: I haz the power, 2015-02-18 14:26:15]

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

[commit-bot: I haz the power, 2015-02-18 14:26:16]

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

[Charlie Reis, 2015-02-19 00:33:44]

Thanks for the test.

Looks like there may be some WebNavigationApiTest failures?  Otherwise still
LGTM.

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl_unittest.cc (left):

https://codereview.chromium.org/903043002/diff/100001/content/browser/frame_h...
content/browser/frame_host/navigator_impl_unittest.cc:275:
main_test_rfh()->SendBeforeUnloadACK(true);
On 2015/02/18 13:27:23, clamy wrote:
> On 2015/02/17 23:04:31, Charlie Reis wrote:
> > We should include a PlzNavigate test for the case that a beforeunload
handler
> is
> > present as well.
> 
> I added a test to check that we properly wait before starting the navigation
if
> a handler is present as well.

Acknowledged.