[Prerender] first contentful paint histograms.

chrome/browser/prerender/prerender_manager.h

Index: chrome/browser/prerender/prerender_manager.h
diff --git a/chrome/browser/prerender/prerender_manager.h b/chrome/browser/prerender/prerender_manager.h
index ee4bf7f2ff1fb9cb782c31ddd4a59ef4c8c6da70..3ac49e451f08aad5e97006cb6b246fdebbdc22fa 100644
--- a/chrome/browser/prerender/prerender_manager.h
+++ b/chrome/browser/prerender/prerender_manager.h
@@ -103,6 +103,12 @@ class PrerenderManager : public content::NotificationObserver,
     CLEAR_MAX = 0x1 << 2
   };
 
+  // Observer interface for PrerenderManager events.
+  class Observer {

[pasko, 2016/11/17 19:52:16]

A toplevel class PrerenderManagerObserver is preferable. Two reasonas I can see:
1. grep-able more easily
2. This class does not need to have access to internal state of the
PrerenderManager, but at the place where it is extended it creates a confusion
that the base class needs access to the private state of PrerenderManager.
Isolating it out is more explicit and hence easier to read (modulo 'friend',
which is discouraged).

[mattcary, 2016/11/18 09:21:11]

Done.

I was copying what PrerenderContents was doing.

+   public:
+    virtual void OnFirstContentfulPaint() = 0;

[pasko, 2016/11/17 19:52:16]

should have a comment saying on which thread all the methods of this class are
called.

[mattcary, 2016/11/18 09:21:11]

Done.

+  };
+
   // Owned by a Profile object for the lifetime of the profile.
   explicit PrerenderManager(Profile* profile);
   ~PrerenderManager() override;
@@ -219,13 +225,18 @@ class PrerenderManager : public content::NotificationObserver,
                                    bool is_main_resource,
                                    int redirect_count);
 
-  // Records the time to first contentful paint.
+  // Records the time to first contentful paint for no state prefetch loads.

[pasko, 2016/11/17 19:52:16]

is it rather for loads that were recently prefetched with nostate prefetch in
the same browsing session?

[mattcary, 2016/11/18 09:21:11]

Done.

   // Must not be called for prefetch loads (which are never rendered anyway).
   // |is_no_store| must be true if the main resource has a "no-store" cache
   // control HTTP header.
-  void RecordFirstContentfulPaint(const GURL& url,
-                                  bool is_no_store,
-                                  base::TimeDelta time);
+  void RecordNoStateFirstContentfulPaint(const GURL& url,
+                                         bool is_no_store,
+                                         base::TimeDelta time);
+
+  // Records the perceived first contentful paint time for a prerendered page,
+  // analogous to |RecordPerceivedPageLoadTime|.
+  void RecordPerceivedFirstContentfulPaint(content::WebContents* web_contents,
+                                           base::TimeDelta time);
 
   static PrerenderManagerMode GetMode();
   static void SetMode(PrerenderManagerMode mode);
@@ -329,6 +340,16 @@ class PrerenderManager : public content::NotificationObserver,
   void SetTickClockForTesting(
       std::unique_ptr<base::SimpleTestTickClock> tick_clock);
 
+  void DisablePageLoadMetricsObserverForTesting() {

[pasko, 2016/11/17 19:52:16]

I see a few tests calling this, does something break if they don't do it?

[mattcary, 2016/11/18 09:21:11]

Yes, if it's not disabled there will be a race between the mock timing in the
test, and the actual timings recorded from the page_load_metrics.

The tests instantiate a PrerenderPageLoadMetricsObserver which fires of the
OnFirstContentfulPaint message manually.

[Bryan McQuade, 2016/11/18 14:52:37]

You do get a bit less complete test coverage this way - by instantiating your
own PrerenderPageLoadMetricsObserver you don't verify that the production code
would have instantiated a PrerenderPageLoadMetricsObserver for this page load,
for example. You also aren't verifying that the expected timing updates are
dispatched to your observer for a page with a contentful paint, though this is
reasonably well tested elsewhere.

Is there something we could do to enable you to rely on the default observer
being instantiated, and not have to create a custom observer?

One thing our other browser tests have done in these cases is assert not that a
specific value of FCP was logged, but just that the FCP histogram did get
logged. We then have separate unit tests to provide specific values and assert
that they get logged. This gives good coverage in both areas: in the browser
test we verify the expected end to end logic as best we can, while in the unit
test we verify the expected behavior of the observer when certain timing events
are passed to it.

I don't feel strongly here - it's up to you - but if there's something we can do
to enable you to avoid needing this method, I'd be happy to try to support that,
and I think it'd lead to slightly better coverage as well, as you'd be testing
that the observer instantiated in production code has the expected value, rather
than creating a fake observer.

[mattcary, 2016/11/18 15:14:13]

We already have logging of the histogram counts elsewhere (or will have it;
there's other CLs in flight). 

The point of these tests is specifically the timing, as Egor pointed out earlier
on in this CL, so that we can confirm, eg, when there are two prefetches the
perceived contentful paint is attributed to the correct load and so the timed
interval is correct.
I basically agree with you; I'd prefer to test the actual observer chain too.
However, it seems very difficult to initialize the observer with a custom class
that could override the timings (and I think it's necessary to replace the
observer, otherwise we race with actual timing updates). The only solution I can
see would involve creating some global singleton for PrerenderPLMO to use in
CreateIfNeeded for injection, and that approach has been rejected elsewhere.

I'm very possibly missing a solution, though, so any suggestions are welcome!

Note also that it's not feasible to test this sort of prerender other than in a
browser test. I tried for a couple of days to get a unittest together, in the
style of those found in page_load_metrics/observers/, and one ends up on a path
to mocking out all of chrome.

+    page_load_metric_observer_disabled_ = true;
+  }
+
+  bool PageLoadMetricsObserverDisabledForTesting() const {
+    return page_load_metric_observer_disabled_;
+  }
+
+  void AddObserver(std::unique_ptr<Observer> observer);
+
   // Notification that a prerender has completed and its bytes should be
   // recorded.
   void RecordNetworkBytes(Origin origin, bool used, int64_t prerender_bytes);
@@ -610,6 +631,10 @@ class PrerenderManager : public content::NotificationObserver,
   std::unique_ptr<base::Clock> clock_;
   std::unique_ptr<base::TickClock> tick_clock_;
 
+  bool page_load_metric_observer_disabled_;
+
+  std::vector<std::unique_ptr<Observer>> observers_;
+
   base::WeakPtrFactory<PrerenderManager> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(PrerenderManager);