Factor PageLoadTracker into its own header and impl files.

Factor PageLoadTracker into its own header and impl files.

metrics_web_contents_observer.h/.cc have become quite complex.
This change factors the PageLoadTracker code into its own files,
in order to make each individual file more manageable.

Committed: https://crrev.com/0563b1858d6c4e0ddcb6c08d75917651fff6dd70
Cr-Commit-Position: refs/heads/master@{#427216}

[Bryan McQuade, 2016-10-21 02:37:38]

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

[commit-bot: I haz the power, 2016-10-21 02:38:02]

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-10-21 02:46:38]

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

[commit-bot: I haz the power, 2016-10-21 02:46:57]

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-10-21 03:04:55]

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

[commit-bot: I haz the power, 2016-10-21 03:04:56]

Dry run: Try jobs failed on following builders:
  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, 2016-10-21 03:11:04]

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

[commit-bot: I haz the power, 2016-10-21 03:11:30]

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-10-21 03:27:47]

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

[commit-bot: I haz the power, 2016-10-21 03:27:47]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Bryan McQuade, 2016-10-21 16:54:46]

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

[Bryan McQuade, 2016-10-21 16:57:46]

PTAL, thanks!

This was motivated by the fact that I'm pointing some people on Nat Duca's team
towards page_load_metrics as a potential foundation for building a more generic
metrics tracking system. I want to point them at MWCO but there is so much in
that file that I think it'll be overwhelming. Breaking this out helps to reduce
the overhead of browsing / learning the code.

One slight issue here is that there are 3 helper methods:

void RecordInternalError(InternalErrorLoadEvent event);
UserAbortType AbortTypeForPageTransition(ui::PageTransition transition);
void LogAbortChainSameURLHistogram(int aborted_chain_size_same_url);

which are needed by both PLT and MWCO, and which it's very difficult to fix such
that only one file needs them. For now, I declared them in the
page_load_tracker.h file and allowed MWCO to invoke them, which seems ok. Longer
term it'd be nicer to find a way to break the sharing, but it's not clear that's
possible.

[Charlie Harrison, 2016-10-22 02:00:01]

High level this looks good (have not dug into the nitty gritty). We should have
done this a long time ago :)

I wonder if we can nuke the embedder interface though. It was designed when this
was in //components but now I feel like the abstraction doesn't make sense?
WDYT?

[Bryan McQuade, 2016-10-24 15:42:20]

On 2016/10/22 at 02:00:01, csharrison wrote:
> High level this looks good (have not dug into the nitty gritty). We should
have done this a long time ago :)
> 
> I wonder if we can nuke the embedder interface though. It was designed when
this was in //components but now I feel like the abstraction doesn't make sense?
WDYT?

Yep, that's a nice cleanup step that I can do before this one. Will go ahead and
remove the embedder interface first - will send a CL out for that shortly.
Thanks!

[Bryan McQuade, 2016-10-24 15:48:05]

On 2016/10/24 at 15:42:20, Bryan McQuade wrote:
> On 2016/10/22 at 02:00:01, csharrison wrote:
> > High level this looks good (have not dug into the nitty gritty). We should
have done this a long time ago :)
> > 
> > I wonder if we can nuke the embedder interface though. It was designed when
this was in //components but now I feel like the abstraction doesn't make sense?
WDYT?
> 
> Yep, that's a nice cleanup step that I can do before this one. Will go ahead
and remove the embedder interface first - will send a CL out for that shortly.
Thanks!

Actually, the embedder interface makes writing MWCO unit and browser tests
possible - we override various methods, including RegisterObservers, to aid in
testing. So I think having this class makes sense. There's probably some
opportunity to rename it, though conceptually the name "embedder" is actually
pretty reasonable if you consider that tests are also an embedder.

Take a look and see if you see a path to removing the class while making it
straightforward to write tests, or if you have a suggestion for renaming it.

Thanks!
-Bryan

[Charlie Harrison, 2016-10-24 16:32:40]

LGTM w/ nits. Thanks for making the cleanup.

I'm not sure the best way forward w/ embedded interface, so let's punt that
decision to another day.

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h
(right):

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h:30:
#endif  //
CHROME_BROWSER_PAGE_LOAD_METRICS_PAGE_LOAD_METRICS_EMBEDDER_INTERFACE_H_
_H_

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:289:
num_network_requests_(0),
Can you retain the comment above IsNavigationUserInitiated?

[Charlie Harrison, 2016-10-24 20:38:13]

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h
(right):

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_metrics_embedder_interface.h:30:
#endif  //
CHROME_BROWSER_PAGE_LOAD_METRICS_PAGE_LOAD_METRICS_EMBEDDER_INTERFACE_H_
On 2016/10/24 16:32:39, Charlie Harrison wrote:
> _H_

Nevermind, my mistake.

[Bryan McQuade, 2016-10-24 20:42:27]

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

[Bryan McQuade, 2016-10-24 20:43:34]

Thanks! PTAL.

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
File chrome/browser/page_load_metrics/page_load_tracker.cc (right):

https://codereview.chromium.org/2435233002/diff/40001/chrome/browser/page_loa...
chrome/browser/page_load_metrics/page_load_tracker.cc:289:
num_network_requests_(0),
On 2016/10/24 at 16:32:39, Charlie Harrison wrote:
> Can you retain the comment above IsNavigationUserInitiated?

Ah, sorry, I made IsNavigationUserInitiated a function in the header and called
it here and from MWCO.cc.

[commit-bot: I haz the power, 2016-10-24 20:43:44]

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-10-24 20:56:05]

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

[commit-bot: I haz the power, 2016-10-24 20:56:07]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Charlie Harrison, 2016-10-24 20:57:56]

thanks, still lgtm

[Bryan McQuade, 2016-10-25 00:04:03]

The CQ bit was checked by bmcquade@chromium.org

[commit-bot: I haz the power, 2016-10-25 00:04:37]

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-10-25 01:30:38]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-25 01:34:04]

Description was changed from

==========
Factor PageLoadTracker into its own header and impl files.

metrics_web_contents_observer.h/.cc have become quite complex.
This change factors the PageLoadTracker code into its own files,
in order to make each individual file more manageable.
==========

to

==========
Factor PageLoadTracker into its own header and impl files.

metrics_web_contents_observer.h/.cc have become quite complex.
This change factors the PageLoadTracker code into its own files,
in order to make each individual file more manageable.

Committed: https://crrev.com/0563b1858d6c4e0ddcb6c08d75917651fff6dd70
Cr-Commit-Position: refs/heads/master@{#427216}
==========

[commit-bot: I haz the power, 2016-10-25 01:34:05]

Patchset 4 (id:??) landed as
https://crrev.com/0563b1858d6c4e0ddcb6c08d75917651fff6dd70
Cr-Commit-Position: refs/heads/master@{#427216}