PageLoadMetricsObservers observe individual page loads

components/page_load_metrics/browser/metrics_web_contents_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_WEB_CONTENTS_OBSERVER_H_
 #define COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_
 
 #include <map>
 
 #include "base/macros.h"
@@ skipping 99 matching lines @@
   // This event captures all the other ways a provisional load can fail.
   PROVISIONAL_LOAD_ERR_FAILED_NON_ABORT,
 
   // Counts the number of successful commits.
   PROVISIONAL_LOAD_COMMITTED,
 
   // Add values before this final count.
   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.
 //  - Are not navigations to an error page.
 // We only know these things about a navigation post-commit.
 //
 // 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
 // reflect actual errors that occur during a page load.
 //
 // 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 InternalErrorLoadEvent {
   // A timing IPC was sent from the renderer that did not line up with previous
   // data we've received (i.e. navigation start is different or the timing
   // struct is somehow invalid). This error can only occur once the IPC is
   // vetted in other ways (see other errors).
   ERR_BAD_TIMING_IPC,
 
   // The following IPCs are not mutually exclusive.
   //
   // 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,
 
   // 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
   // occur if the browser filters loads less aggressively than the renderer.
   ERR_NO_IPCS_RECEIVED,
 
   // Add values before this final count.
   ERR_LAST_ENTRY
 };
 
 // This class serves as a functional interface to various chrome// features.
 // Impl version is defined in chrome/browser/page_load_metrics.
 class PageLoadMetricsEmbedderInterface {
  public:
   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 /
 // provisional load, until a new navigation commits or the navigation fails. It
 // 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();
   void WebContentsShown();
 
   // 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.
   const GURL& GetCommittedURL();
 
   base::TimeDelta GetBackgroundDelta();
   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.
   const base::TimeTicks navigation_start_;
 
   // We record separate metrics for events that occur after a background,
   // because metrics like layout/paint are delayed artificially
   // when they occur in the background.
   base::TimeTicks background_time_;
   base::TimeTicks foreground_time_;
   bool started_in_foreground_;
 
   PageLoadTiming timing_;
   GURL url_;
 
   // Interface to chrome features. Must outlive the class.
   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);
 };
 
 // MetricsWebContentsObserver logs page load UMA metrics based on
 // 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
   // outlive the WebContents.
   static MetricsWebContentsObserver* CreateForWebContents(
       content::WebContents* web_contents,
       scoped_ptr<PageLoadMetricsEmbedderInterface> embedder_interface);
   MetricsWebContentsObserver(
       content::WebContents* web_contents,
       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;
   void DidStartNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidFinishNavigation(
       content::NavigationHandle* navigation_handle) override;
   void DidRedirectNavigation(
       content::NavigationHandle* navigation_handle) override;
 
@@ skipping 12 matching lines @@
 
   // This map tracks all of the navigations ongoing that are not committed
   // yet. Once a navigation is committed, it moves from the map to
   // committed_load_. Note that a PageLoadTrackers NavigationHandle is only
   // valid until commit time, when we remove it from the map.
   std::map<content::NavigationHandle*, scoped_ptr<PageLoadTracker>>
       provisional_loads_;
   scoped_ptr<PageLoadTracker> committed_load_;
 
   scoped_ptr<PageLoadMetricsEmbedderInterface> embedder_interface_;
-  base::ObserverList<PageLoadMetricsObserver, true> observers_;
 
   DISALLOW_COPY_AND_ASSIGN(MetricsWebContentsObserver);
 };
 
 }  // namespace page_load_metrics
 
 #endif  // COMPONENTS_PAGE_LOAD_METRICS_BROWSER_PAGE_LOAD_METRICS_WEB_CONTENTS_OBSERVER_H_