(DEPRECATED) Send navigation_start to browser process in DidStartProvisionalLoad

Send navigation_start to browser process in DidStartProvisionalLoad IPC

This CL has been split into three:
https://codereview.chromium.org/1432583002/
https://codereview.chromium.org/1423453007/
https://codereview.chromium.org/1427633004/

BUG=548335

[Charlie Harrison, 2015-10-27 22:03:18]

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

[Charlie Harrison, 2015-10-27 22:03:19]

clamy@, could you take a look at this? I'm not sure this is the most elegant way
to do this, but I mimicked what you did with setUiStartTime on the
WebURLRequest. In patchset #2 I had blink setting it's navigation_start based on
the WebURLRequest too (no need to UpdateTimings after the load is issued), but I
reverted it because I felt it might be a layering violation.

A few questions:
- Why do we only update blinks navigation_start on WebFrameLoadType::Standard?
To me it seems like anything browser initiated should get the updated value.
- I tried to implement this for PlzNavigate too, but I was unsure of how a
browser-initiated navigation uses the same BeginNavigation codepath in the
renderer.

Let me know what you think, thanks!

[Charlie Harrison, 2015-10-27 22:03:53]

Description was changed from

==========
Send navigation_start to browser process in DidStartProvisionalLoad IPC

BUG=
==========

to

==========
Send navigation_start to browser process in DidStartProvisionalLoad IPC

BUG=548335
==========

[clamy, 2015-10-28 12:40:01]

Thanks! A few comments that should help simplify this a bit.

I don't know why we only update the NavigationTiming API when the navigation
type is standard. NavigateInternal is a refactoring of previous functions which
had that behavior so I just kept it in place. Code archeology/contacting the
person in charge of the API may help you there.

For PlzNavigate, the whole thing can be made simpler by having a base::TimeTicks
value in CommonNavigationParams which are used by all navigations.

https://codereview.chromium.org/1425823002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_handle_impl.h (right):

https://codereview.chromium.org/1425823002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_handle_impl.h:208: double
navigation_start_;
This should be a base::TimeTicks or a base::Time (preferably base::TimeTicks).

https://codereview.chromium.org/1425823002/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.h (right):

https://codereview.chromium.org/1425823002/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_request.h:131: void
CreateNavigationHandle(double navigation_start);
There is no need to pass the navigation_start as a parameter since you store all
the NavigationParams which have the relevant information.

https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:737: double /* navigation_start */)
We should send a base::TimeTicks or a base::Time. Timing are only converted to
doubles because of blink. Whenever we get them back, we should convert them to a
proper timing primitive.

https://codereview.chromium.org/1425823002/diff/40001/content/common/navigati...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1425823002/diff/40001/content/common/navigati...
content/common/navigation_params.h:131: double renderer_navigation_start);
Instead of adding a new value in BeginNavigationParams, I suggest we move
browser_navigation_start out of the RequestNavigationParams into the
CommonNavigationParams and rename it navigation_start.

Whether in PlzNavigate or in browser-initiated navigations in the current
architecture, we always initialize a CommonNavigationParams at the beginning of
the navigation. For renderer-initiated navigations in the current architecture,
it's initialized when we create the provisional DataSource for the frame, which
(I think) corresponds to the start of the navigation.

https://codereview.chromium.org/1425823002/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1425823002/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:44: virtual double
GetNavigationStart() = 0;
This should be a base::TimeTicks (or a base::Time if it really can't be
converted to a base::TimeTicks).

https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:462:
request.setBrowserNavigationStartTime(navigation_start_seconds);
Instead of adding the start time to the WebURLRequest, can we add it to the
RequestExtraData? This is a content/class, so we can keep the timing in
base::TimeTicks without having to convert it back and forth to a double.

https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:4321: routing_id_, common_params.url,
navigation_start.InSecondsF()));
nit: in PlzNavigate, you don't really care about what the renderer sends you
since you already have that information in the browser. Just send a
base::TimeTicks()?

https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebDataSource.h (right):

https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebDataSource.h:109: virtual double
getNavigationStartTime() = 0;
What is the issue with getting the time back as a wall time?

[Charlie Harrison, 2015-10-28 14:45:00]

Thanks for the quick review! Adding the timestamp to RequestExtraData and
sending it with Common nav params for PlzNavigate will definitely make this
simpler.

https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
content/common/frame_messages.h:737: double /* navigation_start */)
On 2015/10/28 12:40:01, clamy wrote:
> We should send a base::TimeTicks or a base::Time. Timing are only converted to
> doubles because of blink. Whenever we get them back, we should convert them to
a
> proper timing primitive.

Agree. I did it this way due to how we get the info from blink. Converting
doubles to TimeTicks is a little unorthodox but I think it's possible.

https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
content/renderer/render_frame_impl.cc:462:
request.setBrowserNavigationStartTime(navigation_start_seconds);
On 2015/10/28 12:40:01, clamy wrote:
> Instead of adding the start time to the WebURLRequest, can we add it to the
> RequestExtraData? This is a content/class, so we can keep the timing in
> base::TimeTicks without having to convert it back and forth to a double.

Good idea! Will do that.

https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebDataSource.h (right):

https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebDataSource.h:109: virtual double
getNavigationStartTime() = 0;
On 2015/10/28 12:40:01, clamy wrote:
> What is the issue with getting the time back as a wall time?

DocumentLoadTiming stores the timing as TimeTicks doubles, but PerformanceTiming
/ WebPerformance converts them to time since epoch for reporting to javascript.
I'll update the comment to be more clear.

[Charlie Harrison, 2015-10-28 15:24:25]

On 2015/10/28 14:45:00, csharrison wrote:
> Thanks for the quick review! Adding the timestamp to RequestExtraData and
> sending it with Common nav params for PlzNavigate will definitely make this
> simpler.
> 
>
https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
> File content/common/frame_messages.h (right):
> 
>
https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
> content/common/frame_messages.h:737: double /* navigation_start */)
> On 2015/10/28 12:40:01, clamy wrote:
> > We should send a base::TimeTicks or a base::Time. Timing are only converted
to
> > doubles because of blink. Whenever we get them back, we should convert them
to
> a
> > proper timing primitive.
> 
> Agree. I did it this way due to how we get the info from blink. Converting
> doubles to TimeTicks is a little unorthodox but I think it's possible.
> 
>
https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
> content/renderer/render_frame_impl.cc:462:
> request.setBrowserNavigationStartTime(navigation_start_seconds);
> On 2015/10/28 12:40:01, clamy wrote:
> > Instead of adding the start time to the WebURLRequest, can we add it to the
> > RequestExtraData? This is a content/class, so we can keep the timing in
> > base::TimeTicks without having to convert it back and forth to a double.
> 
> Good idea! Will do that.
> 
>
https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
> File third_party/WebKit/public/web/WebDataSource.h (right):
> 
>
https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
> third_party/WebKit/public/web/WebDataSource.h:109: virtual double
> getNavigationStartTime() = 0;
> On 2015/10/28 12:40:01, clamy wrote:
> > What is the issue with getting the time back as a wall time?
> 
> DocumentLoadTiming stores the timing as TimeTicks doubles, but
PerformanceTiming
> / WebPerformance converts them to time since epoch for reporting to
javascript.
> I'll update the comment to be more clear.

We can do something like this:
TimeTicks renderer_navigation_start =
TimeTicks::FromInternalValue(ds->getNavigationStartTime() *
Time::kMicrosecondsPerSecond);

This will fudge the double back into an internal int64 value. I think that's the
best we can do. I'd need to go over the casting and division / multiplication to
see how lossy we are here.

[Charlie Harrison, 2015-10-29 00:13:52]

On 2015/10/28 15:24:25, csharrison wrote:
> On 2015/10/28 14:45:00, csharrison wrote:
> > Thanks for the quick review! Adding the timestamp to RequestExtraData and
> > sending it with Common nav params for PlzNavigate will definitely make this
> > simpler.
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
> > File content/common/frame_messages.h (right):
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
> > content/common/frame_messages.h:737: double /* navigation_start */)
> > On 2015/10/28 12:40:01, clamy wrote:
> > > We should send a base::TimeTicks or a base::Time. Timing are only
converted
> to
> > > doubles because of blink. Whenever we get them back, we should convert
them
> to
> > a
> > > proper timing primitive.
> > 
> > Agree. I did it this way due to how we get the info from blink. Converting
> > doubles to TimeTicks is a little unorthodox but I think it's possible.
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
> > content/renderer/render_frame_impl.cc:462:
> > request.setBrowserNavigationStartTime(navigation_start_seconds);
> > On 2015/10/28 12:40:01, clamy wrote:
> > > Instead of adding the start time to the WebURLRequest, can we add it to
the
> > > RequestExtraData? This is a content/class, so we can keep the timing in
> > > base::TimeTicks without having to convert it back and forth to a double.
> > 
> > Good idea! Will do that.
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
> > File third_party/WebKit/public/web/WebDataSource.h (right):
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
> > third_party/WebKit/public/web/WebDataSource.h:109: virtual double
> > getNavigationStartTime() = 0;
> > On 2015/10/28 12:40:01, clamy wrote:
> > > What is the issue with getting the time back as a wall time?
> > 
> > DocumentLoadTiming stores the timing as TimeTicks doubles, but
> PerformanceTiming
> > / WebPerformance converts them to time since epoch for reporting to
> javascript.
> > I'll update the comment to be more clear.
> 
> We can do something like this:
> TimeTicks renderer_navigation_start =
> TimeTicks::FromInternalValue(ds->getNavigationStartTime() *
> Time::kMicrosecondsPerSecond);
> 
> This will fudge the double back into an internal int64 value. I think that's
the
> best we can do. I'd need to go over the casting and division / multiplication
to
> see how lossy we are here.

There might be a problem with using RequestExtraData: we can't properly know if
the ds->request().extraData() is a RequestExtraData in
RenderFrameImpl::didStartProvisionalLoad. Unless there's some other way to get
the RequestExtraData in that method we might have to use the original method. We
could also persist the RequestExtraData* and test for pointer equality. WDYT?

[Charlie Harrison, 2015-10-29 00:43:42]

On 2015/10/29 00:13:52, csharrison wrote:
> On 2015/10/28 15:24:25, csharrison wrote:
> > On 2015/10/28 14:45:00, csharrison wrote:
> > > Thanks for the quick review! Adding the timestamp to RequestExtraData and
> > > sending it with Common nav params for PlzNavigate will definitely make
this
> > > simpler.
> > > 
> > >
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
> > > File content/common/frame_messages.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/common/frame_me...
> > > content/common/frame_messages.h:737: double /* navigation_start */)
> > > On 2015/10/28 12:40:01, clamy wrote:
> > > > We should send a base::TimeTicks or a base::Time. Timing are only
> converted
> > to
> > > > doubles because of blink. Whenever we get them back, we should convert
> them
> > to
> > > a
> > > > proper timing primitive.
> > > 
> > > Agree. I did it this way due to how we get the info from blink. Converting
> > > doubles to TimeTicks is a little unorthodox but I think it's possible.
> > > 
> > >
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
> > > File content/renderer/render_frame_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1425823002/diff/40001/content/renderer/render...
> > > content/renderer/render_frame_impl.cc:462:
> > > request.setBrowserNavigationStartTime(navigation_start_seconds);
> > > On 2015/10/28 12:40:01, clamy wrote:
> > > > Instead of adding the start time to the WebURLRequest, can we add it to
> the
> > > > RequestExtraData? This is a content/class, so we can keep the timing in
> > > > base::TimeTicks without having to convert it back and forth to a double.
> > > 
> > > Good idea! Will do that.
> > > 
> > >
> >
>
https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
> > > File third_party/WebKit/public/web/WebDataSource.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1425823002/diff/40001/third_party/WebKit/publ...
> > > third_party/WebKit/public/web/WebDataSource.h:109: virtual double
> > > getNavigationStartTime() = 0;
> > > On 2015/10/28 12:40:01, clamy wrote:
> > > > What is the issue with getting the time back as a wall time?
> > > 
> > > DocumentLoadTiming stores the timing as TimeTicks doubles, but
> > PerformanceTiming
> > > / WebPerformance converts them to time since epoch for reporting to
> > javascript.
> > > I'll update the comment to be more clear.
> > 
> > We can do something like this:
> > TimeTicks renderer_navigation_start =
> > TimeTicks::FromInternalValue(ds->getNavigationStartTime() *
> > Time::kMicrosecondsPerSecond);
> > 
> > This will fudge the double back into an internal int64 value. I think that's
> the
> > best we can do. I'd need to go over the casting and division /
multiplication
> to
> > see how lossy we are here.
> 
> There might be a problem with using RequestExtraData: we can't properly know
if
> the ds->request().extraData() is a RequestExtraData in
> RenderFrameImpl::didStartProvisionalLoad. Unless there's some other way to get
> the RequestExtraData in that method we might have to use the original method.
We
> could also persist the RequestExtraData* and test for pointer equality. WDYT?

Re: persisting. I guess that's what pending_navigation_params_ are. I think that
might be the simplest solution.

[clamy, 2015-10-29 13:26:48]

You can safely cast request->extraData() back to a RequestExtraData. The
ExtraData is meant for the blink embedder to pass an opaque struct with data,
that the embedder can get back later at any time. The only gotcha with that
method is that you must remember to copy the added new member in the new
RequestExtraData created in RenderFrameImpl::willSendRequest.

However, we also have access to the *NavigationParams at DidStartProvisionalLoad
time, though not necessarily through the pending_navigation_params_. In
RenderFrameImpl::didCreateDataSource, they are moved to the NavigationState,
inside DocumentState from the DataSource. I think this happens before
didStartProvisionalLoad, but you would need to check.

I would favor the second solution (using the stored CommonNavigationParams),
since it introduces less changes to the code.

[Charlie Harrison, 2015-10-29 18:20:29]

I decided to go with the RequestExtraData route. Using the common_params sounded
nice in theory, but which I had to code around in awkward ways in practice. For
instance, on the renderer side, we want the common_params to have nav-start only
if they should actually override with that value. Unfortunately this means that
immediately after setting pending_navigation_params_, we need to set
pending_navigation_params_->common_params->navigation_start = null;

Also, this way we can make the default constructor have nav-start = Now(),
rather than null, which simplifies a lot of tests.

Let me know if you still prefer that method though.

[clamy, 2015-10-30 13:54:48]

Yes I would prefer to go with the CommonNavigationParams method, because I think
it will lead us to greater simplicity in the end. This will require modifying a
little how blink works though. Here's what I have in mind.

1) Remove the markNavigationStartTime from performance timing in blink in favor
of always setting the time from content (for how we mark the start time see
below).
2) CommonNavigationParams timestamp would always be initialized with a value of
Now() (we may have to set it to something else for transfer navigation though
but that is a special case).
3) On browser-initiated navigations you receive the CommonNavigationParams from
the browser in OnNavigate. You can sanitize the timestamp here if needed. You do
not update the navigation timing (see latter for when it is done).
4) On browser and renderer intiated navigations you get a call to
RenderFrameImpl::didCreateDataSource. At this point a NavigationStateImpl will
be created that holds the NavigationParams for the navigation. For a
browser-initiated navigation, the NavigationParams are the
pending_navigation_params_, for a renderer-initiated navigation they are created
on the spot.
5) At this point you set the navigation start in the navigation timing using the
timestamp from the CommonNavigationParams inside the NavigationState. For
browser-initiated navigations this is the timestamp received from the browser
which is what we want. For renderer-initiated navigations, didCreateDataSource
is called from createDocumentLoader. It is called just before
DocumentLoader::StartLoadingMainResource, which currently marks the navigation
start. So we should have the same timing as well.
6) When we get to send the DidStartProvisionalLoad message, send the timestamp
stored in the CommonNavigationParams from the NavigationState. Given 5) we know
it's the same as the navigation start in the timing API.

I think this should remove some of the complexity surrounding the navigation
start time.

https://codereview.chromium.org/1425823002/diff/60001/content/public/renderer...
File content/public/renderer/navigation_state.h (right):

https://codereview.chromium.org/1425823002/diff/60001/content/public/renderer...
content/public/renderer/navigation_state.h:10: #include "base/time/time.h"
Unnecessary include.

[Charlie Harrison, 2015-10-30 14:04:46]

On 2015/10/30 13:54:48, clamy wrote:
> Yes I would prefer to go with the CommonNavigationParams method, because I
think
> it will lead us to greater simplicity in the end. This will require modifying
a
> little how blink works though. Here's what I have in mind.
> 
> 1) Remove the markNavigationStartTime from performance timing in blink in
favor
> of always setting the time from content (for how we mark the start time see
> below).
> 2) CommonNavigationParams timestamp would always be initialized with a value
of
> Now() (we may have to set it to something else for transfer navigation though
> but that is a special case).
> 3) On browser-initiated navigations you receive the CommonNavigationParams
from
> the browser in OnNavigate. You can sanitize the timestamp here if needed. You
do
> not update the navigation timing (see latter for when it is done).
> 4) On browser and renderer intiated navigations you get a call to
> RenderFrameImpl::didCreateDataSource. At this point a NavigationStateImpl will
> be created that holds the NavigationParams for the navigation. For a
> browser-initiated navigation, the NavigationParams are the
> pending_navigation_params_, for a renderer-initiated navigation they are
created
> on the spot.
> 5) At this point you set the navigation start in the navigation timing using
the
> timestamp from the CommonNavigationParams inside the NavigationState. For
> browser-initiated navigations this is the timestamp received from the browser
> which is what we want. For renderer-initiated navigations, didCreateDataSource
> is called from createDocumentLoader. It is called just before
> DocumentLoader::StartLoadingMainResource, which currently marks the navigation
> start. So we should have the same timing as well.
> 6) When we get to send the DidStartProvisionalLoad message, send the timestamp
> stored in the CommonNavigationParams from the NavigationState. Given 5) we
know
> it's the same as the navigation start in the timing API.
> 
> I think this should remove some of the complexity surrounding the navigation
> start time.
> 
>
https://codereview.chromium.org/1425823002/diff/60001/content/public/renderer...
> File content/public/renderer/navigation_state.h (right):
> 
>
https://codereview.chromium.org/1425823002/diff/60001/content/public/renderer...
> content/public/renderer/navigation_state.h:10: #include "base/time/time.h"
> Unnecessary include.

sgtm! I didn't think about removing markNavigationStart when I first toyed with
CommonNavigationParams as ground-truth for navigation_start. That'll make things
nice :) Thanks!

[Charlie Harrison, 2015-11-02 16:15:59]

Alright, nav start is in common_params now. Some gotchas:
- I had to special case same-page navigations. This ended up being not too
elegant because didCreateDataSource needed to know if the navigation was same
page, which entailed setting navigation_state()->set_was_within_same_page twice,
once in didNavigationWithinPage, and once in didCreateDataSource.

- Because there is a large class of browser initiated loads that we don't want
to override nav start (reloads, etc), we initially set
pending_navigation_params_ nav start to null until we're sure if its value.

- On didStartProvisionalLoad, there are two choices of timestamps to send to the
browser: the timestamp based on blink's navigationStart coerced into a
TimeTicks, and the TimeTicks from common_params if this is browser-initiated. I
chose to always go with the coerced value from blink out of consistency.

[clamy, 2015-11-02 17:34:35]

Thanks! I did not have time to do a full review today, will continue tomorrow.

https://codereview.chromium.org/1425823002/diff/150001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1425823002/diff/150001/content/common/navigat...
content/common/navigation_params.h:64: const base::TimeTicks& navigation_start);
In which cases are we creating CommonNavigationParams with a navigation_start
value other than base::TimeTicks::Now()? If there are none, let's remove this
argument from the constructor.

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

https://codereview.chromium.org/1425823002/diff/150001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2551: void
RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
Do we also create the DataSource in same-page navigation? I though that this
event corresponded to the creation of a provisional DocumentLoader, which does
not happen in same-page navigation (see FrameLoader::loadInSameDocument).

[Charlie Harrison, 2015-11-02 18:19:10]

https://codereview.chromium.org/1425823002/diff/150001/content/common/navigat...
File content/common/navigation_params.h (right):

https://codereview.chromium.org/1425823002/diff/150001/content/common/navigat...
content/common/navigation_params.h:64: const base::TimeTicks& navigation_start);
On 2015/11/02 17:34:35, clamy wrote:
> In which cases are we creating CommonNavigationParams with a navigation_start
> value other than base::TimeTicks::Now()? If there are none, let's remove this
> argument from the constructor.

A few places:
- ConstructCommonNavigationParams uses it, which is used to initiate nav_start
in NavigatorImpl::NavigateToEntry for both RequestNavigation() and
render_frame_host->Navigate.
- In RenderFrameImpl::BeginNavigation we want to set the nav start timestamp
before we MakeCommonNavigationParams (this one could be argued)

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

https://codereview.chromium.org/1425823002/diff/150001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2551: void
RenderFrameImpl::didCreateDataSource(blink::WebLocalFrame* frame,
On 2015/11/02 17:34:35, clamy wrote:
> Do we also create the DataSource in same-page navigation? I though that this
> event corresponded to the creation of a provisional DocumentLoader, which does
> not happen in same-page navigation (see FrameLoader::loadInSameDocument).

You're right, we don't create a new data source. We explicitly call
didCreateDataSource in didNavigateWithinPage (see the comment there). This is to
make sure we update the NavigationState for same-page navs. I'll take a look to
see if the call is really necessary. Could maybe break up didCreateDataSource
into 2 methods.

[Charlie Harrison, 2015-11-02 23:06:24]

csharrison@chromium.org changed reviewers:
+ jochen@chromium.org, nasko@chromium.org

[Charlie Harrison, 2015-11-02 23:06:25]

Looks like removing the call to didCreateDataSource in didNavigateWithinPage
worked out okay. The logic there (as far as I could tell) relevant to same-page
navs I refactored out into UpdateNavigationState.

[clamy, 2015-11-03 13:33:00]

Thanks! I think this patch is getting a bit big, and covers a bit too much. How
about we split it in 3 along the following lines:
- Moving navigation start_time from RequestNavigationParams to
CommonnavigationParams (with proper initialization).
- Changes to the NavigationTiming API use in the renderer.
- Adding the navigation start time to the NavigationHandle, including sending it
with DidStartProvisionalLoad.

I think it will be easier to review each part separately.

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

https://codereview.chromium.org/1425823002/diff/170001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:2159:
suspended_nav_params_->common_params.navigation_start = proceed_time;
This is somewhat problematic. We pushed the navigation start earlier so that we
capture the time it takes to create a renderer in cross-site navigations. This
just overwrites that time in all cross-site navigations, and it's also different
from what we do in PlzNavigate. Could you add a TODO to ensure that PlzNavigate
and the current architecture measure the navigation start in the same way in the
presence of the BeforeUnload event.

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

https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
content/renderer/render_frame_impl.cc:2728: base::TimeTicks navigation_start =
base::TimeTicks::FromInternalValue(
Since we always set the start time of the DataSource using the value from the
NavigationState, why not use the timestamp from the NavigationState here?

https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4677: // value.
Could we check with Navigation Timing API people whether we should also use the
browser-provided timestamp for reloads and history loads? I think it would make
sense to do so, and the code would be simpler.

https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
content/renderer/render_frame_impl.cc:5016: base::TimeTicks navigation_start =
base::TimeTicks::Now();
I don't think this is needed. None of the functions below take time to execute,
so we can postpone getting the navigation_start time until we construct the
CommonNavigationParams.

[Charlie Harrison, 2015-11-03 14:27:08]

On 2015/11/03 13:33:00, clamy wrote:
> Thanks! I think this patch is getting a bit big, and covers a bit too much.
How
> about we split it in 3 along the following lines:
> - Moving navigation start_time from RequestNavigationParams to
> CommonnavigationParams (with proper initialization).
> - Changes to the NavigationTiming API use in the renderer.
> - Adding the navigation start time to the NavigationHandle, including sending
it
> with DidStartProvisionalLoad.
> 
> I think it will be easier to review each part separately.
> 
>
https://codereview.chromium.org/1425823002/diff/170001/content/browser/frame_...
> File content/browser/frame_host/render_frame_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1425823002/diff/170001/content/browser/frame_...
> content/browser/frame_host/render_frame_host_impl.cc:2159:
> suspended_nav_params_->common_params.navigation_start = proceed_time;
> This is somewhat problematic. We pushed the navigation start earlier so that
we
> capture the time it takes to create a renderer in cross-site navigations. This
> just overwrites that time in all cross-site navigations, and it's also
different
> from what we do in PlzNavigate. Could you add a TODO to ensure that
PlzNavigate
> and the current architecture measure the navigation start in the same way in
the
> presence of the BeforeUnload event.
> 
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:2728: base::TimeTicks navigation_start =
> base::TimeTicks::FromInternalValue(
> Since we always set the start time of the DataSource using the value from the
> NavigationState, why not use the timestamp from the NavigationState here?
> 
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:4677: // value.
> Could we check with Navigation Timing API people whether we should also use
the
> browser-provided timestamp for reloads and history loads? I think it would
make
> sense to do so, and the code would be simpler.
> 
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:5016: base::TimeTicks navigation_start =
> base::TimeTicks::Now();
> I don't think this is needed. None of the functions below take time to
execute,
> so we can postpone getting the navigation_start time until we construct the
> CommonNavigationParams.

Sounds good I'll split this CL into 3 today and close this one. Will send an
email about the Navigation Timing API.

[Charlie Harrison, 2015-11-03 14:34:55]

On 2015/11/03 14:27:08, csharrison wrote:
> On 2015/11/03 13:33:00, clamy wrote:
> > Thanks! I think this patch is getting a bit big, and covers a bit too much.
> How
> > about we split it in 3 along the following lines:
> > - Moving navigation start_time from RequestNavigationParams to
> > CommonnavigationParams (with proper initialization).
> > - Changes to the NavigationTiming API use in the renderer.
> > - Adding the navigation start time to the NavigationHandle, including
sending
> it
> > with DidStartProvisionalLoad.
> > 
> > I think it will be easier to review each part separately.
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/170001/content/browser/frame_...
> > File content/browser/frame_host/render_frame_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/170001/content/browser/frame_...
> > content/browser/frame_host/render_frame_host_impl.cc:2159:
> > suspended_nav_params_->common_params.navigation_start = proceed_time;
> > This is somewhat problematic. We pushed the navigation start earlier so that
> we
> > capture the time it takes to create a renderer in cross-site navigations.
This
> > just overwrites that time in all cross-site navigations, and it's also
> different
> > from what we do in PlzNavigate. Could you add a TODO to ensure that
> PlzNavigate
> > and the current architecture measure the navigation start in the same way in
> the
> > presence of the BeforeUnload event.
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> > content/renderer/render_frame_impl.cc:2728: base::TimeTicks navigation_start
=
> > base::TimeTicks::FromInternalValue(
> > Since we always set the start time of the DataSource using the value from
the
> > NavigationState, why not use the timestamp from the NavigationState here?
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> > content/renderer/render_frame_impl.cc:4677: // value.
> > Could we check with Navigation Timing API people whether we should also use
> the
> > browser-provided timestamp for reloads and history loads? I think it would
> make
> > sense to do so, and the code would be simpler.
> > 
> >
>
https://codereview.chromium.org/1425823002/diff/170001/content/renderer/rende...
> > content/renderer/render_frame_impl.cc:5016: base::TimeTicks navigation_start
=
> > base::TimeTicks::Now();
> > I don't think this is needed. None of the functions below take time to
> execute,
> > so we can postpone getting the navigation_start time until we construct the
> > CommonNavigationParams.
> 
> Sounds good I'll split this CL into 3 today and close this one. Will send an
> email about the Navigation Timing API.

Though I think the reason that reloads/history navs are not allowed is because
we aren't supposed to log nav start until after the unload event is dispatched
on the previous document (for navigation timing v1).

[Charlie Harrison, 2015-11-03 19:20:05]

Description was changed from

==========
Send navigation_start to browser process in DidStartProvisionalLoad IPC

BUG=548335
==========

to

==========
Send navigation_start to browser process in DidStartProvisionalLoad IPC

This CL has been split into three:
https://codereview.chromium.org/1432583002/
https://codereview.chromium.org/1423453007/
https://codereview.chromium.org/1427633004/

BUG=548335
==========