Generalize the reload PLMO to support other transition types.

Generalize the reload PLMO to support other transition types.

This change moves tracking of reloads, forward/back navigations
and new navigations into the core PLMO.

It's not unlikely that other observers may want to track load
metrics broken down by load types, so we annotate load types using
suffixes, rather than PageLoad.Clients.LoadType-style naming, which
would not generalize to other observers.

BUG=607063
Committed: https://crrev.com/861b3c8268b5aecbe17436c737f70be7e8b19832
Cr-Commit-Position: refs/heads/master@{#402579}

[Bryan McQuade, 2016-06-24 20:48:51]

Description was changed from

==========
Generalize the reload PLMO to support other transition types.

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

to

==========
Generalize the reload PLMO to support other transition types.

BUG=607063
==========

[Bryan McQuade, 2016-06-24 20:50:12]

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

[Bryan McQuade, 2016-06-25 11:34:45]

Description was changed from

==========
Generalize the reload PLMO to support other transition types.

BUG=607063
==========

to

==========
Generalize the reload PLMO to support other transition types.

This change moves tracking of reloads, forward/back navigations
and new navigations into the core PLMO.

It's not unlikely that other observers may want to track load
metrics broken down by load types, so we annotate load types using
suffixes, rather than PageLoad.Clients.LoadType-style naming, which
would not generalize to other observers.

BUG=607063
==========

[Takashi Toyoshima, 2016-06-27 10:46:14]

lgtm.

One note from my local test just in case:

This problem seems to exist from the beginning, but when we follow the next
step, wrong navigation type is reported.

1. navigate to page A (new navigation)
2. reload (reload)
3. navigate to page B via anchor tag (new navigation)
4. navigation back (reload!)

If the step 2 was skipped, the navigation type at the step 4 is reported
correctly as "forward back".

[Bryan McQuade, 2016-06-27 11:46:44]

Thanks!

https://codereview.chromium.org/2091353002/diff/280001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):

https://codereview.chromium.org/2091353002/diff/280001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:55:
if (transition & ui::PAGE_TRANSITION_FORWARD_BACK) {
regarding your comment about incorrectly accounting for a forward back as a
reload, i wonder, if we put this test first, before the reload test, if that
resolves the issue. forward/back is a modifier on the core type, so my guess is
that when you go back to a reloaded page, you get reload&fowrard_back. if we
check for the forward/back modifier first, we should handle that case correctly.

it's also arguable that going back to a page that was reloaded should no longer
be reported as a reload, but that's something we can at least work around if we
check forward/back first.

[Bryan McQuade, 2016-06-27 20:40:15]

On 2016/06/27 at 11:46:44, Bryan McQuade wrote:
> Thanks!
> 
>
https://codereview.chromium.org/2091353002/diff/280001/chrome/browser/page_lo...
> File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc
(right):
> 
>
https://codereview.chromium.org/2091353002/diff/280001/chrome/browser/page_lo...
>
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer.cc:55:
if (transition & ui::PAGE_TRANSITION_FORWARD_BACK) {
> regarding your comment about incorrectly accounting for a forward back as a
reload, i wonder, if we put this test first, before the reload test, if that
resolves the issue. forward/back is a modifier on the core type, so my guess is
that when you go back to a reloaded page, you get reload&fowrard_back. if we
check for the forward/back modifier first, we should handle that case correctly.
> 
> it's also arguable that going back to a page that was reloaded should no
longer be reported as a reload, but that's something we can at least work around
if we check forward/back first.

I updated the code to check for forward/back before reload, re-ran your test
scenario, and we now record a forward/back navigation when going back to a page
that had previously been reloaded, which seems more correct.

[Bryan McQuade, 2016-06-27 20:40:59]

bmcquade@chromium.org changed reviewers:
+ isherman@chromium.org, jochen@chromium.org
- csharrison@chromium.org

[Bryan McQuade, 2016-06-27 20:41:30]

jochen, PTAL for chrome_browser.gypi
isherman, PTAL for histograms.xml

[Ilya Sherman, 2016-06-28 03:14:08]

histograms.xml lgtm

[Takashi Toyoshima, 2016-06-28 06:46:51]

The workaround looks nicely reasonable. Still LGTM.

[jochen (gone - plz use gerrit), 2016-06-28 13:13:29]

lgtm

[Bryan McQuade, 2016-06-28 14:17:45]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-06-28 14:18:12]

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-06-28 17:07:05]

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

[commit-bot: I haz the power, 2016-06-28 17:07:07]

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

[Bryan McQuade, 2016-06-28 22:26:29]

The CQ bit was checked by bmcquade@chromium.org

[Bryan McQuade, 2016-06-28 22:26:30]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
isherman@chromium.org, toyoshim@chromium.org
Link to the patchset: https://codereview.chromium.org/2091353002/#ps320001
(title: "rebase")

[commit-bot: I haz the power, 2016-06-28 22:28:01]

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-06-28 22:44:11]

Committed patchset #17 (id:320001)

[commit-bot: I haz the power, 2016-06-28 22:47:12]

Description was changed from

==========
Generalize the reload PLMO to support other transition types.

This change moves tracking of reloads, forward/back navigations
and new navigations into the core PLMO.

It's not unlikely that other observers may want to track load
metrics broken down by load types, so we annotate load types using
suffixes, rather than PageLoad.Clients.LoadType-style naming, which
would not generalize to other observers.

BUG=607063
==========

to

==========
Generalize the reload PLMO to support other transition types.

This change moves tracking of reloads, forward/back navigations
and new navigations into the core PLMO.

It's not unlikely that other observers may want to track load
metrics broken down by load types, so we annotate load types using
suffixes, rather than PageLoad.Clients.LoadType-style naming, which
would not generalize to other observers.

BUG=607063

Committed: https://crrev.com/861b3c8268b5aecbe17436c737f70be7e8b19832
Cr-Commit-Position: refs/heads/master@{#402579}
==========

[commit-bot: I haz the power, 2016-06-28 22:47:14]

Patchset 17 (id:??) landed as
https://crrev.com/861b3c8268b5aecbe17436c737f70be7e8b19832
Cr-Commit-Position: refs/heads/master@{#402579}