PageLoadMetricsObservers observe individual page loads

components/page_load_metrics/browser/metrics_web_contents_observer.h

Index: components/page_load_metrics/browser/metrics_web_contents_observer.h
diff --git a/components/page_load_metrics/browser/metrics_web_contents_observer.h b/components/page_load_metrics/browser/metrics_web_contents_observer.h
index 6bc29040d4d7eb5efa9a29b3a6c13ad1f321cea2..610b2068c02683b315c5eee600fa83e791a0b37f 100644
--- a/components/page_load_metrics/browser/metrics_web_contents_observer.h
+++ b/components/page_load_metrics/browser/metrics_web_contents_observer.h
@@ -117,7 +117,8 @@ enum ProvisionalLoadEvent {
   PROVISIONAL_LOAD_LAST_ENTRY
 };
 
-// CommittedLoadEvents are events that occur on committed loads that we track.
+// CommittedRelevantLoadEvents are events that occur on committed loads that we

[Randy Smith (Not in Mondays), 2015/11/28 22:03:13]

nit: There's a preference in the net stack for avoiding use of the word "we" in
comments.  It's not always clear what the meaning is, especially for non-native
english speakers.  Can you substitute the class that's doing the tracking for
"we" here?

[Charlie Harrison, 2015/11/30 16:39:29]

Yupp. I'm trying to break the habit :P

[Randy Smith (Not in Mondays), 2015/12/01 22:34:03]

But you didn't change the comment; is the habit stronger than you thought?  :-}

[Charlie Harrison, 2015/12/01 22:54:24]

Oops :O fixed.

+// track.
 // Note that we capture events only for committed loads that:
 //  - Are http/https.
 //  - Not same-page navigations.
@@ -127,21 +128,21 @@ enum ProvisionalLoadEvent {
 // If you add elements to this enum, make sure you update the enum
 // value in histograms.xml. Only add elements to the end to prevent
 // inconsistencies between versions.
-enum CommittedLoadEvent {
+enum CommittedRelevantLoadEvent {
   // When a load that eventually commits started. Note we can't log this until
   // commit time, but it represents when the actual page load started. Thus, we
   // only separate this into .Background when a page load starts backgrounded.
-  COMMITTED_LOAD_STARTED,
+  RELEVANT_LOAD_STARTED,
 
   // These two events are disjoint. Sum them to find the total number of
   // committed loads that we end up tracking.
-  COMMITTED_LOAD_FAILED_BEFORE_FIRST_LAYOUT,
-  COMMITTED_LOAD_SUCCESSFUL_FIRST_LAYOUT,
+  RELEVANT_LOAD_FAILED_BEFORE_FIRST_LAYOUT,
+  RELEVANT_LOAD_SUCCESSFUL_FIRST_LAYOUT,
 
   // TODO(csharrison) once first paint metrics are in place, add new events.
 
   // Add values before this final count.
-  COMMITTED_LOAD_LAST_ENTRY
+  RELEVANT_LOAD_LAST_ENTRY
 };
 
 // These errors are internal to the page_load_metrics subsystem and do not
@@ -162,7 +163,7 @@ enum InternalErrorLoadEvent {
   // We received an IPC when we weren't tracking a committed load. This will
   // often happen if we get an IPC from a bad URL scheme (that is, the renderer
   // sent us an IPC from a navigation we don't care about).
-  ERR_IPC_WITH_NO_COMMITTED_LOAD,
+  ERR_IPC_WITH_NO_RELEVANT_LOAD,
 
   // Received a notification from a frame that has been navigated away from.
   ERR_IPC_FROM_WRONG_FRAME,
@@ -170,7 +171,7 @@ enum InternalErrorLoadEvent {
   // We received an IPC even through the last committed url from the browser
   // was not http/s. This can happen with the renderer sending IPCs for the
   // new tab page. This will often come paired with
-  // ERR_IPC_WITH_NO_COMMITTED_LOAD.
+  // ERR_IPC_WITH_NO_RELEVANT_LOAD.
   ERR_IPC_FROM_BAD_URL_SCHEME,
 
   // If we track a navigation, but the renderer sends us no IPCs. This could
@@ -188,6 +189,7 @@ class PageLoadMetricsEmbedderInterface {
   virtual ~PageLoadMetricsEmbedderInterface() {}
   virtual rappor::RapporService* GetRapporService() = 0;
   virtual bool IsPrerendering(content::WebContents* web_contents) = 0;
+  virtual void RegisterObservers(PageLoadMetricsObservable* metrics) = 0;
 };
 
 // This class tracks a given page load, starting from navigation start /
@@ -195,15 +197,14 @@ class PageLoadMetricsEmbedderInterface {
 // also records RAPPOR/UMA about the page load.
 // MetricsWebContentsObserver manages a set of provisional PageLoadTrackers, as
 // well as a committed PageLoadTracker.
-class PageLoadTracker {
+class PageLoadTracker : public PageLoadMetricsObservable {

[Randy Smith (Not in Mondays), 2015/11/28 22:03:13]

I know it's not part of this CL, but why the distinction between interface and
derived class here (or previously)?  PageLoadMetricsObservable isn't a base
class anywhere else that I can see (including tests) and I don't think (please
correct if I'm wrong) the class usefully hides any implementation details from
the consumers; it looks like it's just used within the implementation of the
page_load_metrics.  If I'm right in all that I'm inclined to think it's abstract
without an effect other than making the code confusing to follow.  

[Charlie Harrison, 2015/11/30 16:39:29]

The reason we made PageLoadMetricsObservable was so consumers of the API are
shielded from all the implementation details. Components outside of
PageLoadMetrics are only given an object with AddObserver/RemoveObserver methods
exposed. Let me know if you think there's a better architecture for this.

I actually think it ended up being good. In this case we changed the observee
from MetricsWebContentsObserver to PageLoadTracker without having to edit the
observers.

[Randy Smith (Not in Mondays), 2015/12/01 22:34:03]

So I disagree with this as an architectural choice, but not violently, so ok
:-}.  Having said that, let me just give voice to my bias in case there's
something useful in it.
* I don't expect the concrete class associated with PageLoadMetricsObservable to
change very often; maybe if we get this CL wrong it'll change one more time? 
But even that would surprise me.  So I don't think future proofing against
changes to that concrete class is a strong argument.
* I don't think there's a major goodness in the consumers not being able to see
the object they're observing; there might even be value at some point in them
having access to that object to be able to call other methods on it.  They are
consumers; in the abstract them seeing the object they are consuming makes
sense.  (In practice we've put everything on the observer interface, so this
isn't necessary, I just don't consider it a bad thing).
* "PageLoadMetricsObserveable" is a pretty abstract name--it doesn't really say
what the thing is.  As such, it represents just another concept for anyone
reading and understanding this code to keep in their head.  If there's a value
to having that extra concept, that's ok, but in this case I don't see the value,
so I see the extra class as just mental overhead.

I actually do have a suggestion for a different architecture, but I'll put that
in a top level comment.

  public:
   // Caller must guarantee that the observers and embedder_interface pointers
   // outlives this class.
   PageLoadTracker(bool in_foreground,
                   PageLoadMetricsEmbedderInterface* embedder_interface,
-                  content::NavigationHandle* navigation_handle,
-                  base::ObserverList<PageLoadMetricsObserver, true>* observers);
-  ~PageLoadTracker();
+                  content::NavigationHandle* navigation_handle);
+  ~PageLoadTracker() override;
   void Redirect(content::NavigationHandle* navigation_handle);
   void Commit(content::NavigationHandle* navigation_handle);
   void WebContentsHidden();
@@ -212,9 +213,17 @@ class PageLoadTracker {
   // Returns true if the timing was successfully updated.
   bool UpdateTiming(const PageLoadTiming& timing);
   void RecordProvisionalEvent(ProvisionalLoadEvent event);
-  void RecordCommittedEvent(CommittedLoadEvent event, bool backgrounded);
+  void RecordCommittedEvent(CommittedRelevantLoadEvent event,
+                            bool backgrounded);
   bool HasBackgrounded();
 
+  void SetRendererTracked(bool renderer_tracked);
+  bool RendererTracked() { return renderer_tracked_; }

[Randy Smith (Not in Mondays), 2015/11/28 22:03:13]

Note that by the Google style guide these may be named set_renderer_tracked()
and renderer_tracked() (see
https://google.github.io/styleguide/cppguide.html#Function_Names).  It's not a
"should" it's a "can", so it's up to you, but it's good to be aware of the
option and of how surrounding code names things for consistency (I don't believe
that applies here, but if it does, make sure that your usage pattern on these is
consistent).

[Charlie Harrison, 2015/11/30 16:39:29]

Good point. I'll change it in this case. I changed one other method to fit this
rule (GetCommittedURL => committed_url)

+
+  // PageLoadMetricsObservable implementation
+  void AddObserver(PageLoadMetricsObserver* observer) override;
+  void RemoveObserver(PageLoadMetricsObserver* observer) override;
+
  private:
   PageLoadExtraInfo GetPageLoadMetricsInfo();
   // Only valid to call post-commit.
@@ -224,6 +233,9 @@ class PageLoadTracker {
   void RecordTimingHistograms();
   void RecordRappor();
 
+  // Whether the renderer should be sending timing IPCs to this page load.
+  bool renderer_tracked_;
+
   bool has_commit_;
 
   // The navigation start in TimeTicks, not the wall time reported by Blink.
@@ -243,7 +255,7 @@ class PageLoadTracker {
   PageLoadMetricsEmbedderInterface* const embedder_interface_;
 
   // List of observers. This must outlive the class.

[Randy Smith (Not in Mondays), 2015/11/28 22:03:13]

Is the "outlive" sentence still true/useful now that you've gotten rid of the
indirection?

[Charlie Harrison, 2015/11/30 16:39:29]

Nope. Good catch.

-  base::ObserverList<PageLoadMetricsObserver, true>* observers_;
+  base::ObserverList<PageLoadMetricsObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(PageLoadTracker);
 };
@@ -252,8 +264,7 @@ class PageLoadTracker {
 // IPC messages received from a MetricsRenderFrameObserver.
 class MetricsWebContentsObserver
     : public content::WebContentsObserver,
-      public content::WebContentsUserData<MetricsWebContentsObserver>,
-      public PageLoadMetricsObservable {
+      public content::WebContentsUserData<MetricsWebContentsObserver> {
  public:
   // Note that the returned metrics is owned by the web contents.
   // The caller must guarantee that the RapporService (if non-null) will
@@ -266,9 +277,6 @@ class MetricsWebContentsObserver
       scoped_ptr<PageLoadMetricsEmbedderInterface> embedder_interface);
   ~MetricsWebContentsObserver() override;
 
-  void AddObserver(PageLoadMetricsObserver* observer) override;
-  void RemoveObserver(PageLoadMetricsObserver* observer) override;
-
   // content::WebContentsObserver implementation:
   bool OnMessageReceived(const IPC::Message& message,
                          content::RenderFrameHost* render_frame_host) override;
@@ -301,7 +309,6 @@ class MetricsWebContentsObserver
   scoped_ptr<PageLoadTracker> committed_load_;
 
   scoped_ptr<PageLoadMetricsEmbedderInterface> embedder_interface_;
-  base::ObserverList<PageLoadMetricsObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsWebContentsObserver);
 };