Standardize which page loads are tracked

Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which indicates
whether the given page load is expected to report metrics from the render process.

Many page load observers filter out non-tracked navigations by returning early in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so some
observers forget to do this, which results in them tracking e.g. non-HTML pages, but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, there's a consistent request to
avoid tracking the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics, it
will not invoke additional callbacks such as OnComplete on observers.

Note that certain criteria, such as whether the page is HTML, or an error page,
(or even a same page navigation) are not known until commit, and thus we begin
tracking such page loads in DidStartNavigation but stop tracking at commit time.

BUG=627585
Committed: https://crrev.com/26615f40a69f11cee759a0b546061ea84fff8ca8
Cr-Commit-Position: refs/heads/master@{#405774}

[Bryan McQuade, 2016-07-12 20:28:17]

Description was changed from

==========
Prototype to get rid of 'renderer tracked'.

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

to

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, it sounds like there's not a
desire to track the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change updates DidStartNavigation and
DidFinishNavigation
to check to see if the given NavigationHandle meets any of the above criteria
for
not being tracked, and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

BUG=627585
==========

[Bryan McQuade, 2016-07-12 20:29:06]

Description was changed from

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, it sounds like there's not a
desire to track the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change updates DidStartNavigation and
DidFinishNavigation
to check to see if the given NavigationHandle meets any of the above criteria
for
not being tracked, and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

BUG=627585
==========

to

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, it sounds like there's not a
desire to track the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being
tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

BUG=627585
==========

[Bryan McQuade, 2016-07-14 17:20:49]

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

[commit-bot: I haz the power, 2016-07-14 17:21: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, 2016-07-14 17:23:31]

The CQ bit was unchecked by bmcquade@chromium.org

[Bryan McQuade, 2016-07-14 17:23:39]

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

[commit-bot: I haz the power, 2016-07-14 17:24:05]

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, 2016-07-14 17:51:46]

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

[commit-bot: I haz the power, 2016-07-14 17:51:47]

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

[Bryan McQuade, 2016-07-14 18:46:44]

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

[commit-bot: I haz the power, 2016-07-14 18:47:29]

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

[Bryan McQuade, 2016-07-14 18:53:23]

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

[commit-bot: I haz the power, 2016-07-14 18:54:20]

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, 2016-07-14 19:25:45]

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

[commit-bot: I haz the power, 2016-07-14 19:25:46]

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

[Bryan McQuade, 2016-07-14 19:41:29]

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

[commit-bot: I haz the power, 2016-07-14 19:42:16]

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

[Bryan McQuade, 2016-07-14 20:14:48]

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

[commit-bot: I haz the power, 2016-07-14 20:15:17]

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

[Bryan McQuade, 2016-07-14 20:48:18]

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

[commit-bot: I haz the power, 2016-07-14 20:49:00]

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, 2016-07-14 22:23:03]

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

[commit-bot: I haz the power, 2016-07-14 22:23:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Bryan McQuade, 2016-07-15 01:31:04]

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

[commit-bot: I haz the power, 2016-07-15 01:31:37]

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

[Bryan McQuade, 2016-07-15 01:32:54]

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

[commit-bot: I haz the power, 2016-07-15 01:33:22]

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

[Bryan McQuade, 2016-07-15 01:46:50]

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

[commit-bot: I haz the power, 2016-07-15 01:47:16]

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

[Bryan McQuade, 2016-07-15 01:50:09]

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

[commit-bot: I haz the power, 2016-07-15 01:50:37]

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

[Bryan McQuade, 2016-07-15 01:54:18]

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

[commit-bot: I haz the power, 2016-07-15 01:54:50]

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, 2016-07-15 02:29:56]

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

[commit-bot: I haz the power, 2016-07-15 02:29:59]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-15 12:39:56]

Description was changed from

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, it sounds like there's not a
desire to track the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being
tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

BUG=627585
==========

to

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, there's a consistent request
to
avoid tracking the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being
tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

BUG=627585
==========

[Bryan McQuade, 2016-07-15 12:40:25]

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

[Bryan McQuade, 2016-07-15 12:40:31]

PTAL

[Bryan McQuade, 2016-07-15 12:43:11]

Description was changed from

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, there's a consistent request
to
avoid tracking the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being
tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

BUG=627585
==========

to

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, there's a consistent request
to
avoid tracking the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being
tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

Note that certain criteria, such as whether the page is HTML, or an error page,
(or even a same page navigation) are not known until commit, and thus we begin
tracking such page loads in DidStartNavigation but stop tracking at commit time.

BUG=627585
==========

[Charlie Harrison, 2016-07-15 13:01:17]

Looks good, thanks for the refactor.

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:758:
committed_load_ && tracker)
Can we DCHECK(tracker) here instead if keeping it in the if statement?

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
File components/page_load_metrics/browser/page_load_metrics_observer.h (left):

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_observer.h:132: //
OnTimingUpdate is triggered when an updated PageLoadTiming is
Can you include a comment that none of the On* callbacks will be called for non
tracked pages? OnComplete is not important for a big chunk of observers.

[Bryan McQuade, 2016-07-15 14:13:42]

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

[commit-bot: I haz the power, 2016-07-15 14:14:00]

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

[Bryan McQuade, 2016-07-15 14:14:43]

Thanks! Addressed comment, and I added one question about when we should be
invoking the abort callback during DidFinishNavigation. PTAL.

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:758:
committed_load_ && tracker)
On 2016/07/15 at 13:01:17, csharrison wrote:
> Can we DCHECK(tracker) here instead if keeping it in the if statement?

good catch, thanks! this test was left over from an earlier version of the
refactor and is no longer necessary, so i've removed it w/o a dcheck - the
expectation is that this method should never receive a null tracker (other parts
of the function depend on this being the case already).

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:769:
AbortTypeForPageTransition(navigation_handle->GetPageTransition()),
looking at this bit of code, should we be notifying other loads that they
might've been aborted only for tracked commits, or should we be doing so for
untracked commit as well? for instance, if I have a provisional nav go
http://www.google.com/ and that gets aborted by a nav to a chrome:// navigation
that commits, it seems like we may want to notify the google.com load that it
was aborted (due to the chrome nav's commit), even though the chrome nav isn't
tracked. what do you think?

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
File components/page_load_metrics/browser/page_load_metrics_observer.h (left):

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
components/page_load_metrics/browser/page_load_metrics_observer.h:132: //
OnTimingUpdate is triggered when an updated PageLoadTiming is
On 2016/07/15 at 13:01:17, csharrison wrote:
> Can you include a comment that none of the On* callbacks will be called for
non tracked pages? OnComplete is not important for a big chunk of observers.

Good point, yes, I re-ordered the callbacks and added a comment to note which
callbacks won't be invoked for untracked pages.

[Charlie Harrison, 2016-07-15 14:19:55]

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:758:
committed_load_ && tracker)
On 2016/07/15 14:14:42, Bryan McQuade wrote:
> On 2016/07/15 at 13:01:17, csharrison wrote:
> > Can we DCHECK(tracker) here instead if keeping it in the if statement?
> 
> good catch, thanks! this test was left over from an earlier version of the
> refactor and is no longer necessary, so i've removed it w/o a dcheck - the
> expectation is that this method should never receive a null tracker (other
parts
> of the function depend on this being the case already).

Fair enough, though a DCHECK just solidifies this assumption. No big deal either
way.

https://codereview.chromium.org/2139143002/diff/500001/components/page_load_m...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:769:
AbortTypeForPageTransition(navigation_handle->GetPageTransition()),
On 2016/07/15 14:14:42, Bryan McQuade wrote:
> looking at this bit of code, should we be notifying other loads that they
> might've been aborted only for tracked commits, or should we be doing so for
> untracked commit as well? for instance, if I have a provisional nav go
> http://www.google.com/ and that gets aborted by a nav to a chrome://
navigation
> that commits, it seems like we may want to notify the http://google.com load
that it
> was aborted (due to the chrome nav's commit), even though the chrome nav isn't
> tracked. what do you think?

Oh good catch. Can you actually put this code in the caller, after the check for
an in-page navigation?

We want this to be called for any committed load.

[Bryan McQuade, 2016-07-15 14:36:06]

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

[commit-bot: I haz the power, 2016-07-15 14:36:28]

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

[Bryan McQuade, 2016-07-15 15:02:27]

Moved the abort call to happen for all committed navs. PTAL, thanks!

[Charlie Harrison, 2016-07-15 15:04:04]

Sorry should have mentioned it before but could you add a test which catches
that bug?

i.e. abort a page load with a non html page and make sure we log metrics.

[Bryan McQuade, 2016-07-15 15:29:55]

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

[commit-bot: I haz the power, 2016-07-15 15:30:13]

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

[Bryan McQuade, 2016-07-15 15:37:30]

On 2016/07/15 at 15:04:04, csharrison wrote:
> Sorry should have mentioned it before but could you add a test which catches
that bug?
> 
> i.e. abort a page load with a non html page and make sure we log metrics.

Ah, good idea, thanks! I added a few tests to the aborts test. This actually
uncovered what I think may be a subtle bug: previously, we returned early from
DidStartNavigation if the new nav was for e.g. about:blank, which meant
NotifyAbortedProvisionalLoadsNewNavigation didn't get called to update the most
recently aborted provisonal load to type 'unknown_navigation'. I updated
DidStartNavigation to perform all of the abort logic before deciding whether to
track the navigation via ShouldTrackNavigation. Take a look and see if you think
this seems right to you. Thanks!

[Charlie Harrison, 2016-07-15 15:44:08]

Good find and it lgtm. Thanks for adding the tests.

[commit-bot: I haz the power, 2016-07-15 16:44:04]

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

[commit-bot: I haz the power, 2016-07-15 16:44:04]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-07-15 16:55:06]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-07-15 16:55:48]

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, 2016-07-15 17:00:42]

Committed patchset #29 (id:560001)

[commit-bot: I haz the power, 2016-07-15 17:01:11]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 17:02:13]

Description was changed from

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, there's a consistent request
to
avoid tracking the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being
tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

Note that certain criteria, such as whether the page is HTML, or an error page,
(or even a same page navigation) are not known until commit, and thus we begin
tracking such page loads in DidStartNavigation but stop tracking at commit time.

BUG=627585
==========

to

==========
Standardize which page loads are tracked

The current PLM implementation has a concept of 'renderer tracked' which
indicates
whether the given page load is expected to report metrics from the render
process.

Many page load observers filter out non-tracked navigations by returning early
in
OnComplete if the provided PageLoadTiming is empty, but this is non-obvious so
some
observers forget to do this, which results in them tracking e.g. non-HTML pages,
but
not receiving any metrics for those pages. See
https://bugs.chromium.org/p/chromium/issues/detail?id=627536 for one example.

Based on feedback from other observer implementers, there's a consistent request
to
avoid tracking the following:
1. error pages (4xx, 5xx)
2. non-HTTP/HTTPS pages
3. non-HTML pages

Based on the above, this change gets rid of the 'renderer tracked' intermediate
state, and updates DidStartNavigation and DidFinishNavigation to check to see if
the given NavigationHandle meets any of the above criteria for not being
tracked,
and if so:
* In DidStartNavigation, we do not create a PageLoadTracker
* In DidFinishNavigation, we'll tell the existing PageLoadTracker (if any) that
it
should stop tracking metrics. If a PageLoadTracker has stopped tracking metrics,
it
will not invoke additional callbacks such as OnComplete on observers.

Note that certain criteria, such as whether the page is HTML, or an error page,
(or even a same page navigation) are not known until commit, and thus we begin
tracking such page loads in DidStartNavigation but stop tracking at commit time.

BUG=627585

Committed: https://crrev.com/26615f40a69f11cee759a0b546061ea84fff8ca8
Cr-Commit-Position: refs/heads/master@{#405774}
==========

[commit-bot: I haz the power, 2016-07-15 17:02:14]

Patchset 29 (id:??) landed as
https://crrev.com/26615f40a69f11cee759a0b546061ea84fff8ca8
Cr-Commit-Position: refs/heads/master@{#405774}