Add start/finish navigation to startup metrics.

Add start/finish navigation to startup metrics.

Also add a new abandon reason for navigation errors.

BUG=544138, 525209
Committed: https://crrev.com/98286b67a8d14d977f1be9e509302ba4e47f1130
Cr-Commit-Position: refs/heads/master@{#356851}

[gab, 2015-10-22 16:59:41]

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

[gab, 2015-10-22 16:59:41]

Camille, does that look reasonable to you?

Thanks a lot for your help!
Gab

[clamy, 2015-10-22 17:21:07]

Thanks! I think the patch can be made simpler, see my comments below.

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:223: return;
You may also need to check whether the navigation was aborted or not.
NavigationHandle::HasCommitted will tell you if the navigation committed. If you
need to distinguish between a successful commit and an error page commit, use
NavigationHandle::IsErrorPage.

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:239:
startup_metric_utils::RecordFirstWebContentsMainNavigationFinishedAll(
I don't think this is really needed.

While it's true that there can be at most 2 ongoing navigations in the same
frame, the first one two commit will cancel the other one, exceptin one special
case. The special case is a non user-initiated same-site navigation (a
renderer-initiated one) will not cancel an ongoing cross-site navigation (which
is browser-initiated).

In the case of startup, you don't have a renderer with a page loaded in it so
the special case cannot happen. This means that all navigations will always
finish by the time the first of them commit (there are some subtleties if it's
the cross-site one that finishes first due to having to do a round-trip with the
renderer for the execution of the unload event, but we don't allow the same-site
navigation to commit during that time anyway, so the time difference you could
see between the two DidFinishNavigation is not meaningful).

Since you most likely don't need to check that all navigations are finished, you
don't need to store the NavigationHandles either, making this patch simpler :).

[gab, 2015-10-22 20:37:39]

Thanks for the review, PTAL.

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:223: return;
On 2015/10/22 17:21:07, clamy wrote:
> You may also need to check whether the navigation was aborted or not.
> NavigationHandle::HasCommitted will tell you if the navigation committed. If
you
> need to distinguish between a successful commit and an error page commit, use
> NavigationHandle::IsErrorPage.

Good point, even added it as an explicit early exit criteria like some others we
already have in this file.

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:239:
startup_metric_utils::RecordFirstWebContentsMainNavigationFinishedAll(
On 2015/10/22 17:21:07, clamy wrote:
> I don't think this is really needed.
> 
> While it's true that there can be at most 2 ongoing navigations in the same
> frame, the first one two commit will cancel the other one, exceptin one
special
> case. The special case is a non user-initiated same-site navigation (a
> renderer-initiated one) will not cancel an ongoing cross-site navigation
(which
> is browser-initiated).
> 
> In the case of startup, you don't have a renderer with a page loaded in it so
> the special case cannot happen. This means that all navigations will always
> finish by the time the first of them commit (there are some subtleties if it's
> the cross-site one that finishes first due to having to do a round-trip with
the
> renderer for the execution of the unload event, but we don't allow the
same-site
> navigation to commit during that time anyway, so the time difference you could
> see between the two DidFinishNavigation is not meaningful).
> 
> Since you most likely don't need to check that all navigations are finished,
you
> don't need to store the NavigationHandles either, making this patch simpler
:).

Ah ok, maybe this should be made clearer in WebContentsObserver? i.e. from
someone that doesn't know anything about networking I was worried that this
could matter a lot (i.e.  I thought a "navigation" was to a single host and that
there were many per frame to gather all resources -- though now that I think
about it I guess the navigation brings back the main resource and then the
renderers figure out what other resources are needed from there?).

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:254:
FinishedCollectingMetrics(FinishReason::ABANDON_NAVIGATION);
While I have your attention and expertise, WDYT of this? Is this the right
place/way to catch new navigations?

[clamy, 2015-10-26 17:51:42]

Thanks! A few more comments but it looks nicer :).

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:254:
FinishedCollectingMetrics(FinishReason::ABANDON_NAVIGATION);
On 2015/10/22 20:37:39, gab wrote:
> While I have your attention and expertise, WDYT of this? Is this the right
> place/way to catch new navigations?

There could be an issue with client-side redirects. Conceptually, they belong to
the first navigation, so I don't think we want to discard collecting metrics if
we encounter one of those. I'm not familiar enough with the NavigationController
to know if we will get the notification in that case (creis@ is the persons who
knows best).

If we do get the notification, it may be possible to filter those out by
checking the NavigationType in the LoadCommittedDetails.

If this is not possible/desirable, I would suggest merging the code here with
the one in DidFinishNavigation since they will fire at the same point in time
for successful navigations.

https://codereview.chromium.org/1407093003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:193: if
(navigation_handle->IsInMainFrame()) {
I think you could just DCHECK(navigation_handle->IsInMainFrame()) here.
You should not be getting a subframe navigation before a main frame navigation.

https://codereview.chromium.org/1407093003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:209: if
(navigation_handle->IsInMainFrame()) {
Same here, DCHECK(navigation_handle->IsInMainFrame()) should suffice: you can't
have a subframe navigation start before the main frame navigation has committed.

[gab, 2015-10-27 15:54:39]

Patchset #4 (id:60001) has been deleted

[gab, 2015-10-27 15:56:14]

gab@chromium.org changed reviewers:
+ rkaplow@chromium.org

[gab, 2015-10-27 15:56:15]

Thanks +creis for his opinion on the side-discussion below.

+rkaplow for c/b/metrics OWNERS

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:254:
FinishedCollectingMetrics(FinishReason::ABANDON_NAVIGATION);
On 2015/10/26 17:51:42, clamy wrote:
> On 2015/10/22 20:37:39, gab wrote:
> > While I have your attention and expertise, WDYT of this? Is this the right
> > place/way to catch new navigations?
> 
> There could be an issue with client-side redirects. Conceptually, they belong
to
> the first navigation, so I don't think we want to discard collecting metrics
if
> we encounter one of those. I'm not familiar enough with the
NavigationController
> to know if we will get the notification in that case (creis@ is the persons
who
> knows best).

Thanks, +creis to chime in on details.

From what I understand (NavigationEntryCommitted() +
load_details.is_navigation_to_different_page()) would only kick in when the
history would be modified (i.e. new top-level navigation or back/forward history
interaction), is this correct?

I just tried this with a test URL (https://goo.gl/MznQ97 -- which points to this
review), launching locally with chrome.exe https://goo.gl/MznQ97 results in the
stats being collected (i.e. not unintentionally abandoned here).

(not a blocker for this CL as the code under discussion is already in, will
follow-up if needed)

> 
> If we do get the notification, it may be possible to filter those out by
> checking the NavigationType in the LoadCommittedDetails.
> 
> If this is not possible/desirable, I would suggest merging the code here with
> the one in DidFinishNavigation since they will fire at the same point in time
> for successful navigations.

https://codereview.chromium.org/1407093003/diff/40001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:193: if
(navigation_handle->IsInMainFrame()) {
On 2015/10/26 17:51:42, clamy wrote:
> I think you could just DCHECK(navigation_handle->IsInMainFrame()) here.
> You should not be getting a subframe navigation before a main frame
navigation.

Done.

https://codereview.chromium.org/1407093003/diff/40001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:209: if
(navigation_handle->IsInMainFrame()) {
On 2015/10/26 17:51:42, clamy wrote:
> Same here, DCHECK(navigation_handle->IsInMainFrame()) should suffice: you
can't
> have a subframe navigation start before the main frame navigation has
committed.

Done.

[gab, 2015-10-27 16:59:42]

Description was changed from

==========
Add start/finish navigation to startup metrics.

BUG=544138
==========

to

==========
Add start/finish navigation to startup metrics.

Also add a new abandon reason for navigation errors.

BUG=544138, 525209
==========

[Charlie Reis, 2015-10-27 18:35:21]

creis@chromium.org changed reviewers:
+ creis@chromium.org

[Charlie Reis, 2015-10-27 18:35:21]

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:254:
FinishedCollectingMetrics(FinishReason::ABANDON_NAVIGATION);
On 2015/10/27 15:56:15, gab wrote:
> On 2015/10/26 17:51:42, clamy wrote:
> > On 2015/10/22 20:37:39, gab wrote:
> > > While I have your attention and expertise, WDYT of this? Is this the right
> > > place/way to catch new navigations?
> > 
> > There could be an issue with client-side redirects. Conceptually, they
belong
> to
> > the first navigation, so I don't think we want to discard collecting metrics
> if
> > we encounter one of those. I'm not familiar enough with the
> NavigationController
> > to know if we will get the notification in that case (creis@ is the persons
> who
> > knows best).

You will get NavigationEntryCommitted for both the page that does a client
redirect and the destination page.  There will also be no indication that a
client redirect is coming, because that could be determined by script code at
runtime.  You probably just want to treat the first commit as the point you're
done.

> 
> Thanks, +creis to chime in on details.
> 
> From what I understand (NavigationEntryCommitted() +
> load_details.is_navigation_to_different_page()) would only kick in when the
> history would be modified (i.e. new top-level navigation or back/forward
history
> interaction), is this correct?

You can get NavigationEntryCommitted and to_different_page for top-level
navigations and back/forwards.  Those aren't really equivalent, though. 
Top-level navigations and back/forwards can also cause same-page navigations (so
to_different_page() would be false).


> 
> I just tried this with a test URL (https://goo.gl/MznQ97 -- which points to
this
> review), launching locally with chrome.exe https://goo.gl/MznQ97 results in
the
> stats being collected (i.e. not unintentionally abandoned here).

Ah, that's a server redirect, which is a different case.  We don't commit any
navigation until after a server redirect occurs.

A client redirect means that one page actually commits, and it's then quickly
followed by a separate commit that replaces the first one's NavigationEntry.

Hope that helps.

> 
> (not a blocker for this CL as the code under discussion is already in, will
> follow-up if needed)
> 
> > 
> > If we do get the notification, it may be possible to filter those out by
> > checking the NavigationType in the LoadCommittedDetails.
> > 
> > If this is not possible/desirable, I would suggest merging the code here
with
> > the one in DidFinishNavigation since they will fire at the same point in
time
> > for successful navigations.
>

[rkaplow, 2015-10-27 19:25:30]

lgtm

histograms lgtm

https://codereview.chromium.org/1407093003/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1407093003/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44149: +    [Non-mobile] The reason for
which startup profiling was deemed complete.
I would think desktop would be more clear than non-mobile

[gab, 2015-10-28 01:55:13]

Thanks all, will wait for clamy's review to submit.

Gab

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/20001/chrome/browser/metrics/...
chrome/browser/metrics/first_web_contents_profiler.cc:254:
FinishedCollectingMetrics(FinishReason::ABANDON_NAVIGATION);
On 2015/10/27 18:35:21, Charlie Reis wrote:
> On 2015/10/27 15:56:15, gab wrote:
> > On 2015/10/26 17:51:42, clamy wrote:
> > > On 2015/10/22 20:37:39, gab wrote:
> > > > While I have your attention and expertise, WDYT of this? Is this the
right
> > > > place/way to catch new navigations?
> > > 
> > > There could be an issue with client-side redirects. Conceptually, they
> belong
> > to
> > > the first navigation, so I don't think we want to discard collecting
metrics
> > if
> > > we encounter one of those. I'm not familiar enough with the
> > NavigationController
> > > to know if we will get the notification in that case (creis@ is the
persons
> > who
> > > knows best).
> 
> You will get NavigationEntryCommitted for both the page that does a client
> redirect and the destination page.  There will also be no indication that a
> client redirect is coming, because that could be determined by script code at
> runtime.  You probably just want to treat the first commit as the point you're
> done.

Well, we are collecting first commit, but we're also collecting first paint and
main frame load (and want to avoid collecting those if the user navigates again
before they occur as their occurence post-second-navigation would add a
user-inflicted delay in the metrics and wouldn't measure the right thing).

> 
> > 
> > Thanks, +creis to chime in on details.
> > 
> > From what I understand (NavigationEntryCommitted() +
> > load_details.is_navigation_to_different_page()) would only kick in when the
> > history would be modified (i.e. new top-level navigation or back/forward
> history
> > interaction), is this correct?
> 
> You can get NavigationEntryCommitted and to_different_page for top-level
> navigations and back/forwards.  Those aren't really equivalent, though. 
> Top-level navigations and back/forwards can also cause same-page navigations
(so
> to_different_page() would be false).

As long as it's true one-way I'm okay if it's not equivalent (it just means
there is another edge case where we could gather stats after a navigation which
we're trying to avoid but is already better with this code then without it).

> 
> 
> > 
> > I just tried this with a test URL (https://goo.gl/MznQ97 -- which points to
> this
> > review), launching locally with chrome.exe https://goo.gl/MznQ97 results in
> the
> > stats being collected (i.e. not unintentionally abandoned here).
> 
> Ah, that's a server redirect, which is a different case.  We don't commit any
> navigation until after a server redirect occurs.
> 
> A client redirect means that one page actually commits, and it's then quickly
> followed by a separate commit that replaces the first one's NavigationEntry.

I think that's okay. I don't think we can do much about client redirects. We'll
do our best to catch first paint and main frame load metrics but if the redirect
kicks in before we do, we indeed want to cancel gathering our metrics as, again,
the redirect inflicts unexpected noise in the rendering pipeline (in practice
though I would expect it to be very rare for a user's foreground tab upon
launching Chrome to be pointed at a URL with a client side redirect).

> 
> Hope that helps.

Thanks a lot, overall my assessment is that this does what we want (abandons
gathering metrics when a new top-level navigation is introduced in the rendering
pipeline of the first tab on browser startup -- by the user or by script).

> 
> > 
> > (not a blocker for this CL as the code under discussion is already in, will
> > follow-up if needed)
> > 
> > > 
> > > If we do get the notification, it may be possible to filter those out by
> > > checking the NavigationType in the LoadCommittedDetails.
> > > 
> > > If this is not possible/desirable, I would suggest merging the code here
> with
> > > the one in DidFinishNavigation since they will fire at the same point in
> time
> > > for successful navigations.
> > 
>

https://codereview.chromium.org/1407093003/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1407093003/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44149: +    [Non-mobile] The reason for
which startup profiling was deemed complete.
On 2015/10/27 19:25:30, rkaplow wrote:
> I would think desktop would be more clear than non-mobile

Agreed, done.

[clamy, 2015-10-28 11:55:59]

Thanks! One more comment and it should be good.

https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/first_web_contents_profiler.cc:230: void
FirstWebContentsProfiler::NavigationEntryCommitted(
I think this can be merged inside DidFinishNavigation:
void FirstWebContentsProfiler::DidFinishNavigation(
 if (collected_main_navigation_finished_metric_) {
   if (navigation_handle->HasCommitted() &&
       !navigation_handle->IsSamePage()) {
     FinishedCollectingMetrics(FinishReason::ABANDON_NEW_NAVIGATION);
   }
   return;
 }
...

This should also allow to get rid of initial_entry_committed_.

[gab, 2015-10-28 20:17:38]

Thanks, done, one more question below.

https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/first_web_contents_profiler.cc:230: void
FirstWebContentsProfiler::NavigationEntryCommitted(
On 2015/10/28 11:55:59, clamy wrote:
> I think this can be merged inside DidFinishNavigation:
> void FirstWebContentsProfiler::DidFinishNavigation(
>  if (collected_main_navigation_finished_metric_) {
>    if (navigation_handle->HasCommitted() &&
>        !navigation_handle->IsSamePage()) {
>      FinishedCollectingMetrics(FinishReason::ABANDON_NEW_NAVIGATION);
>    }
>    return;
>  }
> ...
> 
> This should also allow to get rid of initial_entry_committed_.

Awesome, done :-)! (also added IsMainFrame() to the check which I think is
required to not trigger on follow-up sub-navigations finishing)

Curious, what does IsSamePage() mean? The method's documentation is not clear to
me (i.e. I tried refreshing a page and clicking on a link to self in a test
page, both times FinishReason::ABANDON_NEW_NAVIGATION triggered (as desired) but
I expected the !IsSamePage() check to skip those).

[Charlie Reis, 2015-10-28 20:58:26]

https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
File chrome/browser/metrics/first_web_contents_profiler.cc (right):

https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
chrome/browser/metrics/first_web_contents_profiler.cc:230: void
FirstWebContentsProfiler::NavigationEntryCommitted(
On 2015/10/28 20:17:38, gab wrote:
> On 2015/10/28 11:55:59, clamy wrote:
> > I think this can be merged inside DidFinishNavigation:
> > void FirstWebContentsProfiler::DidFinishNavigation(
> >  if (collected_main_navigation_finished_metric_) {
> >    if (navigation_handle->HasCommitted() &&
> >        !navigation_handle->IsSamePage()) {
> >      FinishedCollectingMetrics(FinishReason::ABANDON_NEW_NAVIGATION);
> >    }
> >    return;
> >  }
> > ...
> > 
> > This should also allow to get rid of initial_entry_committed_.
> 
> Awesome, done :-)! (also added IsMainFrame() to the check which I think is
> required to not trigger on follow-up sub-navigations finishing)
> 
> Curious, what does IsSamePage() mean? The method's documentation is not clear
to
> me (i.e. I tried refreshing a page and clicking on a link to self in a test
> page, both times FinishReason::ABANDON_NEW_NAVIGATION triggered (as desired)
but
> I expected the !IsSamePage() check to skip those).

IsSamePage means you haven't left the current document, such as fragment
navigations (#foo), pushState, replaceState, etc.  Reloads don't count because
they cause the document to be replaced (with a new one from the same URL).

clamy@, maybe you could clarify that in navigation_handle.h?

[gab, 2015-10-28 21:00:13]

On 2015/10/28 20:58:26, Charlie Reis wrote:
>
https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
> File chrome/browser/metrics/first_web_contents_profiler.cc (right):
> 
>
https://codereview.chromium.org/1407093003/diff/140001/chrome/browser/metrics...
> chrome/browser/metrics/first_web_contents_profiler.cc:230: void
> FirstWebContentsProfiler::NavigationEntryCommitted(
> On 2015/10/28 20:17:38, gab wrote:
> > On 2015/10/28 11:55:59, clamy wrote:
> > > I think this can be merged inside DidFinishNavigation:
> > > void FirstWebContentsProfiler::DidFinishNavigation(
> > >  if (collected_main_navigation_finished_metric_) {
> > >    if (navigation_handle->HasCommitted() &&
> > >        !navigation_handle->IsSamePage()) {
> > >      FinishedCollectingMetrics(FinishReason::ABANDON_NEW_NAVIGATION);
> > >    }
> > >    return;
> > >  }
> > > ...
> > > 
> > > This should also allow to get rid of initial_entry_committed_.
> > 
> > Awesome, done :-)! (also added IsMainFrame() to the check which I think is
> > required to not trigger on follow-up sub-navigations finishing)
> > 
> > Curious, what does IsSamePage() mean? The method's documentation is not
clear
> to
> > me (i.e. I tried refreshing a page and clicking on a link to self in a test
> > page, both times FinishReason::ABANDON_NEW_NAVIGATION triggered (as desired)
> but
> > I expected the !IsSamePage() check to skip those).
> 
> IsSamePage means you haven't left the current document, such as fragment
> navigations (#foo), pushState, replaceState, etc.  Reloads don't count because
> they cause the document to be replaced (with a new one from the same URL).
> 
> clamy@, maybe you could clarify that in navigation_handle.h?

Okay awesome, then the logic in this CL is exactly what I'm looking for, thanks!

[clamy, 2015-10-29 13:13:38]

Thnaks! Lgtm. I'll keep in mind to improve the comments for IsSamePage to make
it clearer.

[gab, 2015-10-29 14:19:18]

The CQ bit was checked by gab@chromium.org

[gab, 2015-10-29 14:19:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1407093003/#ps160001
(title: "merge NavigationEntryCommitted() logic into DidFinishNavigation()")

[commit-bot: I haz the power, 2015-10-29 14:19:34]

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

[commit-bot: I haz the power, 2015-10-29 15:41:08]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2015-10-29 15:41:57]

Patchset 8 (id:??) landed as
https://crrev.com/98286b67a8d14d977f1be9e509302ba4e47f1130
Cr-Commit-Position: refs/heads/master@{#356851}