Histogram the amount of time spent on HTTP(S) page loads

components/page_load_metrics/browser/page_load_metrics_observer.h

Index: components/page_load_metrics/browser/page_load_metrics_observer.h
diff --git a/components/page_load_metrics/browser/page_load_metrics_observer.h b/components/page_load_metrics/browser/page_load_metrics_observer.h
index 23abda5a0819bf6dd95859243ac50748fd64db4d..05b5370386ee7e333b0df2211b2affa65b460600 100644
--- a/components/page_load_metrics/browser/page_load_metrics_observer.h
+++ b/components/page_load_metrics/browser/page_load_metrics_observer.h
@@ -120,7 +120,8 @@ class PageLoadMetricsObserver {
   // the navigation, but will be destroyed soon after this call. Don't hold a
   // reference to it.
   // Note that this does not get called for same page navigations.
-  virtual void OnCommit(content::NavigationHandle* navigation_handle) {}
+  virtual void OnCommit(content::NavigationHandle* navigation_handle,
+                        bool started_in_foreground) {}

[Charlie Harrison, 2016/04/25 13:20:08]

It makes more sense to thread this in OnStart. In any case, I think you're
sending in |started_in_foreground| from a member that's initialized at OnStart.

Another technique would be to add a method to PageLoadMetricsObserver that
allows querying the bg/fg state at any time. This would possibly be a better
design and clean up the API, but it would mean adding some logic to the base
class of PageLoadMetricsObserver.

[felt, 2016/04/25 21:30:04]

Do you mean making PageLoadMetricsObserver no longer a pure interface? I could
add get_in_foreground and set_in_foreground and have it kept updated
appropriately, but it wouldn't be a pure interface anymore.

[Charlie Harrison, 2016/04/25 21:49:11]

Yeah that's what I meant. I wouldn't be opposed to this approach. Another
approach would be to expose more underlying state to the observer, i.e. give a
"page_load_tracker()" method to them, which could be used to expose more
information. That would also entail giving a concrete impl (or threading it
through callbacks).

None of these approaches really stand out to me as being best, so it may make
sense to do the simplest (thread a bool through OnStart).

+bmcquade who may have API opinions.

[Bryan McQuade, 2016/04/25 22:00:04]

I'm open to either providing a getter for the current visibility state, or
moving the boolean to OnStart. I'm disinclined to provide a getter for
PageLoadTracker since we didn't design that class to be a public part of the API
& would want to spend a bit of time before deciding it was ready to expose.

[felt, 2016/04/26 01:46:28]

I personally prefer moving the boolean to OnStart, to keep the interface pure.
So went ahead and did that.

 
   // OnFailedProvisionalLoad is triggered when a provisional load failed and did
   // not commit. Note that provisional loads that result in downloads or 204s
@@ -128,6 +129,15 @@ class PageLoadMetricsObserver {
   virtual void OnFailedProvisionalLoad(
       content::NavigationHandle* navigation_handle) {}
 
+  // OnHidden is triggered when a page leaves the foreground. It does not fire
+  // when a foreground page is permanently closed; for that, listen to
+  // OnComplete instead.
+  virtual void OnHidden() {}
+
+  // OnShown is triggered when a page is brought to the foreground. It does not
+  // fire when the page first loads; for that, listen for OnCommit instead.

[Bryan McQuade, 2016/04/25 22:00:04]

if we move the boolean to OnStart let's update this comment to point to OnStart
as well.

[felt, 2016/04/26 01:46:29]

Done.

+  virtual void OnShown() {}
+
   // OnComplete is triggered when we are ready to record metrics for this page
   // load. This will happen some time after commit. The PageLoadTiming struct
   // contains timing data and the PageLoadExtraInfo struct contains other useful