Create NavigationHandle after beforeunload with PlzNavigate.

Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org

Review-Url: https://codereview.chromium.org/2901833002
Cr-Commit-Position: refs/heads/master@{#474498}
Committed: https://chromium.googlesource.com/chromium/src/+/080e7966ea2c9603ae48cc8c0e2f227fb5dac1e0

[clamy, 2017-05-23 14:11:01]

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

[commit-bot: I haz the power, 2017-05-23 14:11:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-05-23 15:12:30]

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

[commit-bot: I haz the power, 2017-05-23 15:13:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-05-23 17:14:00]

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

[commit-bot: I haz the power, 2017-05-23 17:14:39]

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-05-23 18:59:29]

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

[commit-bot: I haz the power, 2017-05-23 18:59:30]

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

[clamy, 2017-05-24 11:50:53]

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

[commit-bot: I haz the power, 2017-05-24 11:51:13]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[clamy, 2017-05-24 11:53:42]

Description was changed from

==========
Create NavigationHandle after beforeunload with PlzNavigate.

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

patch from issue 2898013004 at patchset 30001
(http://crrev.com/2898013004#ps30001)
==========

to

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

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

[clamy, 2017-05-24 11:54:13]

Description was changed from

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

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

to

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org
==========

[clamy, 2017-05-24 11:55:19]

Description was changed from

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org
==========

to

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org
==========

[clamy, 2017-05-24 11:55:19]

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

[clamy, 2017-05-24 11:56:21]

clamy@chromium.org changed reviewers:
+ sky@chromium.org

[clamy, 2017-05-24 11:56:36]

@nasko, sky: PTAL

[commit-bot: I haz the power, 2017-05-24 12:52:49]

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

[commit-bot: I haz the power, 2017-05-24 12:52:50]

Dry run: This issue passed the CQ dry run.

[nasko, 2017-05-24 14:20:03]

content/ LGTM with nits.

https://codereview.chromium.org/2901833002/diff/140001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2901833002/diff/140001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1224: void
WaitForLoadsToFinish(int num_to_wait_for) {
nit: Arguably, there are no other callers other than the value of 1, so we might
not need a parameter at all.

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:513: :
navigation_request_->nav_entry_id();
nit: Can't we combine this with the previous if statement?
expected_pending_nav_entry_id can be initialized to the nav_entry_id() and if
the handle is not null, updated to be the pending_nav_entry_id.

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:550: ResetNavigationRequest(false,
true);
nit: Add a comment why we don't need to set the error code on the navigation
handle here - we don't have one before we get the response from the beforeunload
handler.

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

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:367: nav_entry_id_ = 0;
nit: This is already initialized to 0 in the header file, do we need to do it
here too?

[jam, 2017-05-24 14:45:01]

jam@chromium.org changed reviewers:
+ jam@chromium.org
- sky@chromium.org

[jam, 2017-05-24 14:45:01]

chrome/ lgtm

[clamy, 2017-05-24 18:34:38]

Thanks!

https://codereview.chromium.org/2901833002/diff/140001/content/browser/devtoo...
File content/browser/devtools/protocol/devtools_protocol_browsertest.cc (right):

https://codereview.chromium.org/2901833002/diff/140001/content/browser/devtoo...
content/browser/devtools/protocol/devtools_protocol_browsertest.cc:1224: void
WaitForLoadsToFinish(int num_to_wait_for) {
On 2017/05/24 14:20:03, nasko wrote:
> nit: Arguably, there are no other callers other than the value of 1, so we
might
> not need a parameter at all.

Done.

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:513: :
navigation_request_->nav_entry_id();
On 2017/05/24 14:20:03, nasko wrote:
> nit: Can't we combine this with the previous if statement?
> expected_pending_nav_entry_id can be initialized to the nav_entry_id() and if
> the handle is not null, updated to be the pending_nav_entry_id.

Done.

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
content/browser/frame_host/frame_tree_node.cc:550: ResetNavigationRequest(false,
true);
On 2017/05/24 14:20:03, nasko wrote:
> nit: Add a comment why we don't need to set the error code on the navigation
> handle here - we don't have one before we get the response from the
beforeunload
> handler.

Done.

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

https://codereview.chromium.org/2901833002/diff/140001/content/browser/frame_...
content/browser/frame_host/navigation_request.cc:367: nav_entry_id_ = 0;
On 2017/05/24 14:20:03, nasko wrote:
> nit: This is already initialized to 0 in the header file, do we need to do it
> here too?

Ah I missed the header file initialization (not used to it so much). Indeed this
is unnecessary. I have removed the else clause.

[clamy, 2017-05-24 18:35:12]

The CQ bit was checked by clamy@chromium.org

[clamy, 2017-05-24 18:35:13]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2901833002/#ps160001
(title: "Addressed comments")

[commit-bot: I haz the power, 2017-05-24 18:35:58]

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-05-24 20:00:24]

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

[commit-bot: I haz the power, 2017-05-24 20:00:25]

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

[nasko, 2017-05-24 21:02:58]

The CQ bit was checked by nasko@chromium.org

[commit-bot: I haz the power, 2017-05-24 21:04:00]

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-05-24 22:15:28]

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

[commit-bot: I haz the power, 2017-05-24 22:15:29]

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

[jam, 2017-05-24 22:22:37]

Description was changed from

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org
==========

to

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org, jam@chromium.org
==========

[jam, 2017-05-24 22:33:53]

Description was changed from

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org, jam@chromium.org
==========

to

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org
==========

[jam, 2017-05-24 22:37:07]

The CQ bit was checked by jam@chromium.org

[jam, 2017-05-24 22:37:07]

The patchset sent to the CQ was uploaded after l-g-t-m from jam@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2901833002/#ps180001
(title: "small fix from jam")

[commit-bot: I haz the power, 2017-05-24 22:38:03]

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-05-25 00:44:21]

CQ is committing da patch.

Bot data: {"patchset_id": 180001, "attempt_start_ts": 1495665427223920,
"parent_rev": "b6dd6525ce6d24e9d3713d639eefb9e372095bde", "commit_rev":
"080e7966ea2c9603ae48cc8c0e2f227fb5dac1e0"}

[commit-bot: I haz the power, 2017-05-25 00:44:33]

Description was changed from

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org
==========

to

==========
Create NavigationHandle after beforeunload with PlzNavigate.

This CL ensures that browser-initiated navigations see their navigation
start time updated to the proceed time of the BeforeUnload event. This
is the correct time to use as per the Navigation Timing API (see
https://www.w3.org/TR/navigation-timing/#dom-performancetiming-navigationstart).

It also ensures that the  NavigationHandle will always be created after the
BeforeUnload event has run when PlzNavigate is enabled (this is already the
case without PlzNavigate). The NavigationHandle will then always have the
correct navigation start time when Plznavigate is enabled.

BUG=705559
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
COLLABORATOR=nasko@chromium.org

Review-Url: https://codereview.chromium.org/2901833002
Cr-Commit-Position: refs/heads/master@{#474498}
Committed:
https://chromium.googlesource.com/chromium/src/+/080e7966ea2c9603ae48cc8c0e2f...
==========

[commit-bot: I haz the power, 2017-05-25 00:44:35]

Committed patchset #10 (id:180001) as
https://chromium.googlesource.com/chromium/src/+/080e7966ea2c9603ae48cc8c0e2f...