Correctly record and pass around navigation start time.

Correctly record and pass around navigation start time.

BUG=None

Committed: https://crrev.com/fb512b00806b1c2163f9419d3e661ada126519ad
Cr-Commit-Position: refs/heads/master@{#354336}

[yzshen1, 2015-10-14 22:10:06]

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

[yzshen1, 2015-10-14 22:10:06]

Hi, Scott.

Would you please take a look? Thanks!

[sky, 2015-10-14 22:46:25]

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
File components/web_view/frame.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
components/web_view/frame.cc:350: navigation_start_time_ =
Does navigation_start_time_ need to be a member? Can it be passed to
OnCanNavigationFrame?

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/pub...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/pub...
components/web_view/public/interfaces/frame.mojom:143: int64
navigation_start_time_ticks) => ();
Document what navigation_start_time_ticks is.

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
File components/web_view/url_request_cloneable.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
components/web_view/url_request_cloneable.cc:77: request->originating_time_ticks
= base::TimeTicks::Now().ToInternalValue();
Why do you want to set the time here? If no one supplied a time shouldn't it
remain null?

https://codereview.chromium.org/1391963004/diff/20001/mojo/services/network/p...
File mojo/services/network/public/interfaces/url_loader.mojom (right):

https://codereview.chromium.org/1391963004/diff/20001/mojo/services/network/p...
mojo/services/network/public/interfaces/url_loader.mojom:39: // The time when
this request is originated. 0 indicates that it is not
'is originated' -> originated

[yzshen1, 2015-10-14 22:59:16]

Thanks Scott. Please take another look.

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
File components/web_view/frame.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
components/web_view/frame.cc:350: navigation_start_time_ =
On 2015/10/14 22:46:24, sky wrote:
> Does navigation_start_time_ need to be a member? Can it be passed to
> OnCanNavigationFrame?

In that case we also need to pass it to ChangeClient, OnWillNavigateAck,
InitClient. I feel that it seems easier to have a member variable. What do you
think? Thanks!

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/pub...
File components/web_view/public/interfaces/frame.mojom (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/pub...
components/web_view/public/interfaces/frame.mojom:143: int64
navigation_start_time_ticks) => ();
On 2015/10/14 22:46:24, sky wrote:
> Document what navigation_start_time_ticks is.

Done.

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
File components/web_view/url_request_cloneable.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
components/web_view/url_request_cloneable.cc:77: request->originating_time_ticks
= base::TimeTicks::Now().ToInternalValue();
On 2015/10/14 22:46:24, sky wrote:
> Why do you want to set the time here? If no one supplied a time shouldn't it
> remain null?

When we do back/forward navigation, we clone the original request and use it to
start a new navigation. We want "now" to be the navigation start time.

https://codereview.chromium.org/1391963004/diff/20001/mojo/services/network/p...
File mojo/services/network/public/interfaces/url_loader.mojom (right):

https://codereview.chromium.org/1391963004/diff/20001/mojo/services/network/p...
mojo/services/network/public/interfaces/url_loader.mojom:39: // The time when
this request is originated. 0 indicates that it is not
On 2015/10/14 22:46:24, sky wrote:
> 'is originated' -> originated

Done. English is hard.

[sky, 2015-10-14 23:08:36]

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
File components/web_view/frame.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
components/web_view/frame.cc:350: navigation_start_time_ =
On 2015/10/14 22:59:15, yzshen1 wrote:
> On 2015/10/14 22:46:24, sky wrote:
> > Does navigation_start_time_ need to be a member? Can it be passed to
> > OnCanNavigationFrame?
> 
> In that case we also need to pass it to ChangeClient, OnWillNavigateAck,
> InitClient. I feel that it seems easier to have a member variable. What do you
> think? Thanks!

The problem with the member is that it's easy to accidentally reset and end up
using the wrong value. By passing the value around it's clear the value
corresponds to a particular navigation.

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
File components/web_view/url_request_cloneable.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
components/web_view/url_request_cloneable.cc:77: request->originating_time_ticks
= base::TimeTicks::Now().ToInternalValue();
On 2015/10/14 22:59:16, yzshen1 wrote:
> On 2015/10/14 22:46:24, sky wrote:
> > Why do you want to set the time here? If no one supplied a time shouldn't it
> > remain null?
> 
> When we do back/forward navigation, we clone the original request and use it
to
> start a new navigation. We want "now" to be the navigation start time.

Can't the code that clones it reset the time?

[yzshen1, 2015-10-15 00:05:10]

Thanks Scott!

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
File components/web_view/frame.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/fra...
components/web_view/frame.cc:350: navigation_start_time_ =
On 2015/10/14 23:08:36, sky wrote:
> On 2015/10/14 22:59:15, yzshen1 wrote:
> > On 2015/10/14 22:46:24, sky wrote:
> > > Does navigation_start_time_ need to be a member? Can it be passed to
> > > OnCanNavigationFrame?
> > 
> > In that case we also need to pass it to ChangeClient, OnWillNavigateAck,
> > InitClient. I feel that it seems easier to have a member variable. What do
you
> > think? Thanks!
> 
> The problem with the member is that it's easy to accidentally reset and end up
> using the wrong value. By passing the value around it's clear the value
> corresponds to a particular navigation.

Done.

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
File components/web_view/url_request_cloneable.cc (right):

https://codereview.chromium.org/1391963004/diff/20001/components/web_view/url...
components/web_view/url_request_cloneable.cc:77: request->originating_time_ticks
= base::TimeTicks::Now().ToInternalValue();
On 2015/10/14 23:08:36, sky wrote:
> On 2015/10/14 22:59:16, yzshen1 wrote:
> > On 2015/10/14 22:46:24, sky wrote:
> > > Why do you want to set the time here? If no one supplied a time shouldn't
it
> > > remain null?
> > 
> > When we do back/forward navigation, we clone the original request and use it
> to
> > start a new navigation. We want "now" to be the navigation start time.
> 
> Can't the code that clones it reset the time?

Done.

[sky, 2015-10-15 13:14:46]

Can you write a test that verifies times?

https://codereview.chromium.org/1391963004/diff/80001/components/web_view/nav...
File components/web_view/navigation_entry.cc (right):

https://codereview.chromium.org/1391963004/diff/80001/components/web_view/nav...
components/web_view/navigation_entry.cc:17: mojo::URLRequestPtr
NavigationEntry::BuildURLRequest() {
Instead of implying whether the time is reset based on state can
NavigationController explicitly specify what it wants to happen? I believe
NavigationController knows back/forward vs new.

[yzshen1, 2015-10-15 18:23:08]

Also added an app test case.

Please take another look. Thanks!

https://codereview.chromium.org/1391963004/diff/80001/components/web_view/nav...
File components/web_view/navigation_entry.cc (right):

https://codereview.chromium.org/1391963004/diff/80001/components/web_view/nav...
components/web_view/navigation_entry.cc:17: mojo::URLRequestPtr
NavigationEntry::BuildURLRequest() {
On 2015/10/15 13:14:46, sky wrote:
> Instead of implying whether the time is reset based on state can
> NavigationController explicitly specify what it wants to happen? I believe
> NavigationController knows back/forward vs new.

Done.

[sky, 2015-10-15 20:00:52]

LGTM

[yzshen1, 2015-10-15 20:16:12]

The CQ bit was checked by yzshen@chromium.org

[commit-bot: I haz the power, 2015-10-15 20:17:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1391963004/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1391963004/100001

[commit-bot: I haz the power, 2015-10-15 20:23:25]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-10-15 20:24:11]

Patchset 6 (id:??) landed as
https://crrev.com/fb512b00806b1c2163f9419d3e661ada126519ad
Cr-Commit-Position: refs/heads/master@{#354336}