Updating histograms to allow for experiments & log origin-based histograms.

chrome/browser/prerender/prerender_manager.h

Index: chrome/browser/prerender/prerender_manager.h
===================================================================
--- chrome/browser/prerender/prerender_manager.h	(revision 91002)
+++ chrome/browser/prerender/prerender_manager.h	(working copy)
@@ -67,6 +67,9 @@
     CLEAR_MAX = 0x1 << 2
   };
 
+  // ID indicating that no experiment is active.
+  static const char kNoExperiment = -1;

[dominich, 2011/06/30 18:11:15]

A uint8 set to static_cast<uint8>(-1) if you agree to use uint8 for the type. I
wonder if an enum is even better in this case.

[tburkard, 2011/06/30 19:45:48]

Done.

+
   // Owned by a Profile object for the lifetime of the profile.
   PrerenderManager(Profile* profile, PrerenderTracker* prerender_tracker);
 
@@ -185,6 +188,11 @@
   // Intended to be used when clearing the cache or history.
   void ClearData(int clear_flags);
 
+  // Record a final status of a prerendered page in a histogram.
+  void RecordFinalStatus(Origin origin,
+                         char experiment_id,
+                         FinalStatus final_status) const;
+
  protected:
   // Test that needs needs access to internal functions.
   FRIEND_TEST_ALL_PREFIXES(PrerenderManagerTest, ExpireTest);
@@ -205,6 +213,12 @@
   static bool MaybeGetQueryStringBasedAliasURL(const GURL& url,
                                                GURL* alias_url);
 
+  // Extracts an experiment stored in the query parameter

[cbentzel, 2011/06/30 18:15:39]

Can we just combine this with MaybeGetQueryStringBasedAliasURL?

I'd also consider moving it to a separate file and just make it a top-level
function, rather than a static on PrernederManager. 

[tburkard, 2011/06/30 19:45:48]

Since it's a local check that only prerendermanager needs to perform, I think it
makes sense to make it part of that class.  Would you prefer it being a full
member as opposed to static?

On 2011/06/30 18:15:39, cbentzel wrote:

[cbentzel, 2011/06/30 19:51:28]

The reason to move it out is the prerender_manager.[h|cc] are pretty big. Easier
to read and test if top-level function that this includes. I also noticed that
the new method is not tested. 

+  // lpe= from the URL supplied, and returns it.
+  // Returns kNoExperiment if no experiment ID is found, or if the ID
+  // is not an integer in the range 0 to 9.
+  static char GetQueryStringBasedExperiment(const GURL& url);
+
   base::TimeDelta max_prerender_age() const;
   unsigned int max_elements() const;
 
@@ -258,7 +272,8 @@
   virtual base::TimeTicks GetCurrentTimeTicks() const;
   virtual PrerenderContents* CreatePrerenderContents(const GURL& url,
                                                      const GURL& referrer,
-                                                     Origin origin);
+                                                     Origin origin,
+                                                     char experiment_id);
 
   // Checks if the PrerenderContents has been added to the pending delete list.
   bool IsPendingDelete(PrerenderContents* entry) const;
@@ -322,6 +337,22 @@
   // navigates to it. This must be called on the UI thread.
   void RecordTimeUntilUsed(base::TimeDelta time_until_used);
 
+  // Composes a histogram name based on a histogram type.

[dominich, 2011/06/30 18:11:15]

It doesn't look like these really need to be in the PrerenderManager. Can we
have a PrerenderHistogram or PrerenderExperimentTracker class that manages this
stuff?

[tburkard, 2011/06/30 19:45:48]

Can I do the cleanup in a separate CL?  This CL is already complex, I'd rather
get in the functionality first, and then do a more straightforward cleanup.

On 2011/06/30 18:11:15, dominich wrote:

+  std::string ComposeHistogramName(std::string prefix_type,
+                                   std::string name) const;
+
+  // Returns the histogram name for a given origin and experiment.
+  std::string GetHistogramName(Origin origin, char experiment_id,
+                               std::string name) const;
+  // Returns the histogram name for the current window.
+  std::string GetDefaultHistogramName(std::string name) const;
+  // Returns the current experiment.
+  char GetCurrentExperimentId() const;
+  // Returns the current origin.
+  Origin GetCurrentOrigin() const;
+  // Returns whether or not there is currently an origin/experiment wash.
+  bool IsOriginExperimentWash() const;
+
   // Specifies whether prerendering is currently enabled for this
   // manager. The value can change dynamically during the lifetime
   // of the PrerenderManager.
@@ -363,6 +394,18 @@
 
   static PrerenderManagerMode mode_;
 
+  // An integer indicating a Prerendering Experiment being currently conducted.
+  // (The last experiment ID seen).
+  char last_experiment_id_;
+
+  // Origin of the last prerender seen.
+  Origin last_origin_;
+
+  // A boolean indicating that we have recently encountered a combination of
+  // different experiments and origins, making an attribution of PPLT's to
+  // experiments / origins impossible.
+  bool origin_experiment_wash_;
+
   // The time when we last saw a prerender request coming from a renderer.
   // This is used to record perceived PLT's for a certain amount of time
   // from the point that we last saw a <link rel=prerender> tag.