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

components/page_load_metrics/browser/page_load_metrics_observer.h

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_OBSERVER_H_
 #define COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_OBSERVER_H_
 
 #include "base/macros.h"
 #include "components/page_load_metrics/common/page_load_timing.h"
 #include "content/public/browser/navigation_handle.h"
@@ skipping 102 matching lines @@
   // The navigation handle holds relevant data for the navigation, but will
   // be destroyed soon after this call. Don't hold a reference to it. This can
   // be called multiple times.
   virtual void OnRedirect(content::NavigationHandle* navigation_handle) {}
 
   // OnCommit is triggered when a page load commits, i.e. when we receive the
   // first data for the request. The navigation handle holds relevant data for
   // 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
   // are aborted by the system, and thus considered failed provisional loads.
   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
   // data collected over the course of the page load. If the load did not
   // receive any timing information, |timing.IsEmpty()| will be true.
   // After this call, the object will be deleted.
   virtual void OnComplete(const PageLoadTiming& timing,
                           const PageLoadExtraInfo& extra_info) {}
 };
 
 }  // namespace page_load_metrics
 
 #endif  // COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_OBSERVER_H_