Generalize abort tracking to page end state tracking

Generalize abort tracking to page end state tracking

page_load_metrics abort tracking combines tracking of some
page 'end state' information, such as navigating away from
the page or closing the tab, as well as some intermediate
page state information, such as backgrounding the page.

this makes the implementation complex and sometimes
difficult to reason about.

this change refactors abort tracking into tracking page
end states, which abort observers can convert into
PageAbortInfo which provides a more abort-centric view
of the page load.

BUG=693725
Review-Url: https://codereview.chromium.org/2699933003
Cr-Commit-Position: refs/heads/master@{#452036}
Committed: https://chromium.googlesource.com/chromium/src/+/909ebae98693013efbeb783a143c02c5bf2214f3

[Bryan McQuade, 2017-02-16 22:41:43]

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

[commit-bot: I haz the power, 2017-02-16 22:43:15]

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-02-16 23:08:25]

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

[commit-bot: I haz the power, 2017-02-16 23:08:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Bryan McQuade, 2017-02-17 04:45:43]

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

[commit-bot: I haz the power, 2017-02-17 04:46:21]

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-02-17 06:25:29]

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

[commit-bot: I haz the power, 2017-02-17 06:25:30]

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

[Bryan McQuade, 2017-02-17 17:12:24]

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

[commit-bot: I haz the power, 2017-02-17 17:13:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 17:40:18]

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

[commit-bot: I haz the power, 2017-02-17 17:40:32]

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-02-17 17:55:22]

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

[commit-bot: I haz the power, 2017-02-17 17:55:23]

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

[Bryan McQuade, 2017-02-17 18:07:34]

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

[commit-bot: I haz the power, 2017-02-17 18:08:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 18:13:53]

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

[commit-bot: I haz the power, 2017-02-17 18:14:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 18:42:38]

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

[commit-bot: I haz the power, 2017-02-17 18:42:50]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 19:05:48]

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

[commit-bot: I haz the power, 2017-02-17 19:06:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 20:31:35]

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

[commit-bot: I haz the power, 2017-02-17 20:33:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 21:11:20]

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

[commit-bot: I haz the power, 2017-02-17 21:12:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 21:15:18]

Description was changed from

==========
Migrate from AbortType to PageEndReason

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

to

==========
Generalize abort tracking to page end state tracking

page_load_metrics abort tracking combines tracking of some
page 'end state' information, such as navigating away from
the page or closing the tab, as well as some intermediate
page state information, such as backgrounding the page.

this makes the implementation complex and sometimes
difficult to reason about.

this change refactors abort tracking into tracking page
end states, which abort observers can convert into
PageAbortInfo which provides a more abort-centric view
of the page load.

this also generalizes abort time tracking to track time
on page.

this change introduces new 'time on page' metrics which
allow us to understand the total time users spend on
pages, as well as time spent on pages after first paint.

BUG=693725
==========

[Bryan McQuade, 2017-02-17 21:33:28]

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

[commit-bot: I haz the power, 2017-02-17 21:33:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 21:55:13]

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

[commit-bot: I haz the power, 2017-02-17 21:56:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 22:01:28]

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

[commit-bot: I haz the power, 2017-02-17 22:01:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-17 22:12:06]

Description was changed from

==========
Generalize abort tracking to page end state tracking

page_load_metrics abort tracking combines tracking of some
page 'end state' information, such as navigating away from
the page or closing the tab, as well as some intermediate
page state information, such as backgrounding the page.

this makes the implementation complex and sometimes
difficult to reason about.

this change refactors abort tracking into tracking page
end states, which abort observers can convert into
PageAbortInfo which provides a more abort-centric view
of the page load.

this also generalizes abort time tracking to track time
on page.

this change introduces new 'time on page' metrics which
allow us to understand the total time users spend on
pages, as well as time spent on pages after first paint.

BUG=693725
==========

to

==========
Generalize abort tracking to page end state tracking

page_load_metrics abort tracking combines tracking of some
page 'end state' information, such as navigating away from
the page or closing the tab, as well as some intermediate
page state information, such as backgrounding the page.

this makes the implementation complex and sometimes
difficult to reason about.

this change refactors abort tracking into tracking page
end states, which abort observers can convert into
PageAbortInfo which provides a more abort-centric view
of the page load.

BUG=693725
==========

[Bryan McQuade, 2017-02-17 22:12:34]

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

[Bryan McQuade, 2017-02-17 22:12:38]

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

[Bryan McQuade, 2017-02-17 22:12:57]

Here is the abort -> endreason refactor. PTAL, thanks!

[commit-bot: I haz the power, 2017-02-17 22:13:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-02-17 22:36:49]

First pass, mostly ignored mechanical changes.

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc
(left):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:311:

Why remove the DCHECK?

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (left):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:442:
IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, AbortClientRedirect) {
Please keep this test ans just assert we don't log any aborts.

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:173: // * a new
navigation which later commits is initiated in the same tab
Please give an example of when/why this will *not* be set, even if it is a bug.

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:322: if
(started_in_foreground_) {
I kinda think we should just set page_end_time_ = Now() here.

[commit-bot: I haz the power, 2017-02-18 00:15:20]

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

[commit-bot: I haz the power, 2017-02-18 00:15:21]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[Bryan McQuade, 2017-02-18 00:30:16]

Thanks! Addressed comments. PTAL.

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc
(left):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/aborts_page_load_metrics_observer.cc:311:

On 2017/02/17 at 22:36:49, Charlie Harrison wrote:
> Why remove the DCHECK?

Ah, I changed the API a bit so time_to_abort isn't an optional field in
PageAbortInfo - it's considered an invalid field when the reason is ABORT_NONE

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc (left):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_browsertest.cc:442:
IN_PROC_BROWSER_TEST_F(PageLoadMetricsBrowserTest, AbortClientRedirect) {
On 2017/02/17 at 22:36:49, Charlie Harrison wrote:
> Please keep this test ans just assert we don't log any aborts.

Done

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:173: // * a new
navigation which later commits is initiated in the same tab
On 2017/02/17 at 22:36:49, Charlie Harrison wrote:
> Please give an example of when/why this will *not* be set, even if it is a
bug.

Done

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2699933003/diff/240001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.cc:322: if
(started_in_foreground_) {
On 2017/02/17 at 22:36:49, Charlie Harrison wrote:
> I kinda think we should just set page_end_time_ = Now() here.

Agree, done. I kept the error counter since I do want to know how often this
happens in the field (should be never).

[Bryan McQuade, 2017-02-18 00:30:18]

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

[commit-bot: I haz the power, 2017-02-18 00:31:13]

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-02-18 02:36:45]

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

[commit-bot: I haz the power, 2017-02-18 02:36:46]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Bryan McQuade, 2017-02-18 02:48:30]

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

[commit-bot: I haz the power, 2017-02-18 02:49:14]

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-02-18 04:50:04]

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

[commit-bot: I haz the power, 2017-02-18 04:50:05]

Dry run: Exceeded global retry quota

[Bryan McQuade, 2017-02-20 11:57:16]

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

[commit-bot: I haz the power, 2017-02-20 11:57:33]

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-02-20 13:08:14]

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

[commit-bot: I haz the power, 2017-02-20 13:08:15]

Dry run: This issue passed the CQ dry run.

[Charlie Harrison, 2017-02-21 19:49:31]

LG but I would appreciate a few tests for the new histograms.

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:434: if
(committed_load_) {
Why not propagate this to all loads, including the aborted loads?

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:84:
ERR_END_BEFORE_NAVIGATION_START,
please update the histograms xml file for this.

[Bryan McQuade, 2017-02-21 21:51:17]

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

[Bryan McQuade, 2017-02-21 21:51:59]

Thanks! Addressed comments & added unit tests & browser test. PTAL.

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:434: if
(committed_load_) {
On 2017/02/21 at 19:49:31, Charlie Harrison wrote:
> Why not propagate this to all loads, including the aborted loads?

RenderProcessGone associated with the render frame host for the currently
committed load. We don't know if the pending navs or aborted pending navs are
associated w/ the render process that died, so we can't be sure the info should
propagate to them. I added a comment for this.

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_tracker.h (right):

https://codereview.chromium.org/2699933003/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_tracker.h:84:
ERR_END_BEFORE_NAVIGATION_START,
On 2017/02/21 at 19:49:31, Charlie Harrison wrote:
> please update the histograms xml file for this.

Done, thanks!

[commit-bot: I haz the power, 2017-02-21 21:52:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Bryan McQuade, 2017-02-21 21:53:32]

bmcquade@chromium.org changed reviewers:
+ isherman@chromium.org

[Bryan McQuade, 2017-02-21 21:53:47]

isherman, PTAL for histograms.xml, thanks!

[Charlie Harrison, 2017-02-21 21:55:10]

LGTM w/ nit

https://codereview.chromium.org/2699933003/diff/300001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/metrics_web_contents_observer.cc (right):

https://codereview.chromium.org/2699933003/diff/300001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/metrics_web_contents_observer.cc:436: //
provisional / aborted provisional loads.
nit: I think your comment on the issue is clearer than this comment. I think the
key point here is that we don't know which render process is going to host the
provisional load (esp after PlzNavigate).

[Ilya Sherman, 2017-02-21 23:08:29]

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

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44480: +    being backgrounded, for page
loads that started in the foreground.
Is this only recorded for pages that were ever backgrounded, or is some special
value recorded for pages that stayed in the foreground indefinitely?  What's
recorded if I open a page and don't close it for a month, during which time my
computer goes to sleep multiple times (i.e. base::TimeTicks pauses)?  (If the
timing is limited to just the page load, then I guess that question is probably
irrelevant, but it's a bit unclear -- maybe because the context of "what page
loads is this metric recorded for?" is not entirely clear.)

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44509: +    navigations that started in
the background.
What is recorded if the user never foregrounds the tab?  What if the user waits
a month to foreground it?

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:95464: +  <int value="5" label="Page end
reported before navigation start"/>
Hmm, why did this label change?  Is it a semantic change, or just a
clarification of what the semantics were all along?

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:117790: +  <suffix name="NoEndTime"
label="The page load had no recorded end time."/>
Hmm, this sounds like the metric is recording (\inf - t), where t is the first
paint or navigation time.  Is the value always recorded into the overflow
bucket?  I'm not really understanding what sort of data I would expect to see
for histograms with this suffix, nor what it would mean...

[commit-bot: I haz the power, 2017-02-21 23:58:22]

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

[commit-bot: I haz the power, 2017-02-21 23:58:23]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[Bryan McQuade, 2017-02-22 00:57:18]

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

[Bryan McQuade, 2017-02-22 00:57:33]

Thanks! I addressed comments. PTAL, thanks!

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

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44480: +    being backgrounded, for page
loads that started in the foreground.
On 2017/02/21 at 23:08:29, Ilya Sherman wrote:
> Is this only recorded for pages that were ever backgrounded, or is some
special value recorded for pages that stayed in the foreground indefinitely? 
What's recorded if I open a page and don't close it for a month, during which
time my computer goes to sleep multiple times (i.e. base::TimeTicks pauses)? 
(If the timing is limited to just the page load, then I guess that question is
probably irrelevant, but it's a bit unclear -- maybe because the context of
"what page loads is this metric recorded for?" is not entirely clear.)

This is only recorded for pages that were ever backgrounded.

This, along with the two other new histograms in this change
'FirstPaintToPageEnd' and 'FirstPaintToPageEnd.NoEndTime' help us to understand
how often these 3 cases of page termination are encountered for page loads
processed by the page load metrics infrastructure.

This is recorded for the entire duration from when a page paints to when it
first goes into the background. This histogram will have a very large tail,
which is expected / intended. Happy to try to clarify this in the name or
description - just make suggestions for the text & I can try to incorporate it.

RE: base::TimeTicks pauses, these are general issues that affect all histograms
that use TimeTicks (we have many), so I'm disinclined to mention this in this
histogram. In this case, the pause during suspend behavior is desirable - this
metric is a proxy for the time that a user spends on a page, so time spent
suspended shouldn't be counted.

Given that the other 2 histograms are in PageLoad.Experimental, I've moved this
one to PageLoad.Experimental as well. The goal here for the time being is to get
some data to understand how often these 3 cases are encountered (as well as what
the histogram distribution looks like), as a first step towards building a
better non-experimental histogram that can give insight into the time users
spend on web pages.

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44509: +    navigations that started in
the background.
On 2017/02/21 at 23:08:29, Ilya Sherman wrote:
> What is recorded if the user never foregrounds the tab?  What if the user
waits a month to foreground it?

This histogram will have a long tail, which is fine. This histogram only logs
metrics for pages that start in the background and are eventually foregrounded.
This is just a rename of the existing
PageLoad.Timing2.NavigationToFirstForeground - we've wanted to get rid of
Timing2 for a while but didn't have a good category name to put this one in -
the 'PageTiming' category is a better name & home for this one.

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:95464: +  <int value="5" label="Page end
reported before navigation start"/>
On 2017/02/21 at 23:08:29, Ilya Sherman wrote:
> Hmm, why did this label change?  Is it a semantic change, or just a
clarification of what the semantics were all along?

There is no behavior change, but the backing implementation details changed to
use the name 'page end' in some places where we used the name 'abort'
previously, so this keeps the histogram name in sync with the code.

https://codereview.chromium.org/2699933003/diff/300001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:117790: +  <suffix name="NoEndTime"
label="The page load had no recorded end time."/>
On 2017/02/21 at 23:08:29, Ilya Sherman wrote:
> Hmm, this sounds like the metric is recording (\inf - t), where t is the first
paint or navigation time.  Is the value always recorded into the overflow
bucket?  I'm not really understanding what sort of data I would expect to see
for histograms with this suffix, nor what it would mean...

Yeah, this is a bad description, thanks for catching it! I've updated it to be a
little clearer. We want to understand how frequently this case is happening and
whether its histogram shape differs from cases where an end time was explicitly
recorded. This is an experimental histogram (PageLoad.Experimental) and I expect
to fold it into a simpler non-experimental histogram once we gather data and
learn a bit more about what we see in the field.

[commit-bot: I haz the power, 2017-02-22 00:58:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Ilya Sherman, 2017-02-22 01:33:13]

Metrics LGTM, thanks for the clarifications!

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

https://codereview.chromium.org/2699933003/diff/320001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:44226: +    being backgrounded, for page
loads that started in the foreground.
nit: I'd add "and were backgrounded prior to page load completing" (or something
like that) to the end of this sentence.  And, to keep the sentence from getting
too complex, I might split the sentence into two, like so: "... up until being
backgrounded.  Recorded only for page loads that ..."

[Bryan McQuade, 2017-02-22 01:47:30]

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

[commit-bot: I haz the power, 2017-02-22 01:49:10]

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-02-22 03:51:25]

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

[commit-bot: I haz the power, 2017-02-22 03:51:27]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[Bryan McQuade, 2017-02-22 11:38:26]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2017-02-22 11:38:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
csharrison@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2699933003/#ps360001
(title: "fix comment")

[commit-bot: I haz the power, 2017-02-22 11:38:44]

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

[Bryan McQuade, 2017-02-22 11:45:00]

The CQ bit was unchecked by bmcquade@chromium.org

[Bryan McQuade, 2017-02-22 11:45:12]

bmcquade@chromium.org changed reviewers:
+ mattcary@chromium.org

[Bryan McQuade, 2017-02-22 11:45:40]

mattcary, PTAL for small change to
chrome/browser/prerender/prerender_browsertest.cc, thanks!

[Bryan McQuade, 2017-02-22 11:45:55]

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

[commit-bot: I haz the power, 2017-02-22 11:46:13]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mattcary, 2017-02-22 11:54:17]

LGTM for prerender browser test.

There's been a few CLs where the PageLoadExtraInfo creation needs to be tweaked
for the prerender browser test. It's kind of an artificial dependancy. Would it
be useful to move GeneraicPageLoadExtraInfo into page_load_metrics/ or somesuch?

[Bryan McQuade, 2017-02-22 11:55:18]

On 2017/02/22 at 11:54:17, mattcary wrote:
> LGTM for prerender browser test.
> 
> There's been a few CLs where the PageLoadExtraInfo creation needs to be
tweaked for the prerender browser test. It's kind of an artificial dependancy.
Would it be useful to move GeneraicPageLoadExtraInfo into page_load_metrics/ or
somesuch?

Yes, good idea - I don't expect this to happen again soon, but if it does we'll
create a helper method and move to using that. Thanks!

[commit-bot: I haz the power, 2017-02-22 14:00:45]

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

[commit-bot: I haz the power, 2017-02-22 14:00:48]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2017-02-22 14:17:12]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2017-02-22 14:17:33]

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-02-22 14:21:58]

CQ is committing da patch.

Bot data: {"patchset_id": 360001, "attempt_start_ts": 1487773032529320,
"parent_rev": "0e05c9b817a75ba6fa1d24d2a9921121ccae80f9", "commit_rev":
"909ebae98693013efbeb783a143c02c5bf2214f3"}

[commit-bot: I haz the power, 2017-02-22 14:22:32]

Description was changed from

==========
Generalize abort tracking to page end state tracking

page_load_metrics abort tracking combines tracking of some
page 'end state' information, such as navigating away from
the page or closing the tab, as well as some intermediate
page state information, such as backgrounding the page.

this makes the implementation complex and sometimes
difficult to reason about.

this change refactors abort tracking into tracking page
end states, which abort observers can convert into
PageAbortInfo which provides a more abort-centric view
of the page load.

BUG=693725
==========

to

==========
Generalize abort tracking to page end state tracking

page_load_metrics abort tracking combines tracking of some
page 'end state' information, such as navigating away from
the page or closing the tab, as well as some intermediate
page state information, such as backgrounding the page.

this makes the implementation complex and sometimes
difficult to reason about.

this change refactors abort tracking into tracking page
end states, which abort observers can convert into
PageAbortInfo which provides a more abort-centric view
of the page load.

BUG=693725

Review-Url: https://codereview.chromium.org/2699933003
Cr-Commit-Position: refs/heads/master@{#452036}
Committed:
https://chromium.googlesource.com/chromium/src/+/909ebae98693013efbeb783a143c...
==========

[commit-bot: I haz the power, 2017-02-22 14:22:34]

Committed patchset #21 (id:360001) as
https://chromium.googlesource.com/chromium/src/+/909ebae98693013efbeb783a143c...