[Prerender] first contentful paint histograms.

chrome/browser/prerender/prerender_manager.h

 // Copyright (c) 2012 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 CHROME_BROWSER_PRERENDER_PRERENDER_MANAGER_H_
 #define CHROME_BROWSER_PRERENDER_PRERENDER_MANAGER_H_
 
 #include <stdint.h>
 
 #include <memory>
@@ skipping 85 matching lines @@
   };
 
   // One or more of these flags must be passed to ClearData() to specify just
   // what data to clear.  See function declaration for more information.
   enum ClearFlags {
     CLEAR_PRERENDER_CONTENTS = 0x1 << 0,
     CLEAR_PRERENDER_HISTORY = 0x1 << 1,
     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;
 
   // From KeyedService:
   void Shutdown() override;
 
   // Entry points for adding prerenders.
 
   // Adds a prerender for |url| if valid. |process_id| and |route_id| identify
@@ skipping 96 matching lines @@
                                       bool is_main_resource,
                                       bool is_redirect,
                                       bool is_no_store);
 
   // Called when a NoStatePrefetch resource has been loaded. This is called only
   // once per resource, when all redirects have been resolved.
   void RecordPrefetchRedirectCount(Origin origin,
                                    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);
   static bool IsPrerenderingPossible();
   static bool ActuallyPrerendering();
   static bool IsControlGroup();
   static bool IsNoUseGroup();
   static bool IsNoStatePrefetch();
 
   // Query the list of current prerender pages to see if the given web contents
@@ skipping 83 matching lines @@
   Profile* profile() const { return profile_; }
 
   // Return current time and ticks with ability to mock the clock out for
   // testing.
   base::Time GetCurrentTime() const;
   base::TimeTicks GetCurrentTimeTicks() const;
   void SetClockForTesting(std::unique_ptr<base::SimpleTestClock> clock);
   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);
 
   // Add to the running tally of bytes transferred over the network for this
   // profile if prerendering is currently enabled.
   void AddProfileNetworkBytesIfEnabled(int64_t bytes);
 
   // Registers a new ProcessHost performing a prerender. Called by
   // PrerenderContents.
@@ skipping 261 matching lines @@
   // The value of profile_network_bytes_ that was last recorded.
   int64_t last_recorded_profile_network_bytes_;
 
   // Set of process hosts being prerendered.
   using PrerenderProcessSet = std::set<content::RenderProcessHost*>;
   PrerenderProcessSet prerender_process_hosts_;
 
   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);
 };
 
 }  // namespace prerender
 
 #endif  // CHROME_BROWSER_PRERENDER_PRERENDER_MANAGER_H_