Use a different algorithm with the low entropy source for field trials.

base/metrics/field_trial.h

Index: base/metrics/field_trial.h
===================================================================
--- base/metrics/field_trial.h	(revision 152081)
+++ base/metrics/field_trial.h	(working copy)
@@ -94,6 +94,18 @@
  public:
   typedef int Probability;  // Probability type for being selected in a trial.
 
+  // EntropyProvider is an interface for providing entropy for one-time
+  // randomized (persistent) field trials.
+  class BASE_EXPORT EntropyProvider {
+   public:
+    virtual ~EntropyProvider();
+
+    // Returns a double in the range of [0, 1) based on |trial_name| that will
+    // be used for the dice roll for the specified field trial. A given instance
+    // should always return the same value given the same input |trial_name|.
+    virtual double GetEntropyForTrial(const std::string& trial_name) const = 0;
+  };

[jar (doing other things), 2012/08/17 19:06:19]

Please add DISALLOW_COPY_AND_ASSIGN(..)

[Alexei Svitkine (slow), 2012/08/20 15:57:40]

We don't usually do this for pure interface classes. Here's an example search
for "Delegate", which brings up a bunch of these (that don't use the macro):

http://code.google.com/p/chromium/source/search?q=Delegate

Using the macro creates the following problem for subclasses that implement the
interface (i.e. forcing them to declare an empty constructor):

error: constructor for 'SHA1EntropyProvider' must explicitly initialize the base
class 'base::FieldTrial::EntropyProvider' which does not have a default
constructor

+
   // A pair representing a Field Trial and its selected group.
   struct SelectedGroup {
     std::string trial;
@@ -188,7 +200,8 @@
   // consumers don't use it by mistake in cases where the group was forced.
   static const int kDefaultGroupNumber;
 
-  FieldTrial(const std::string& name,
+  FieldTrial(const EntropyProvider* entropy_provider,

[jar (doing other things), 2012/08/17 19:06:19]

nit: add comment that |entropy_provider_| is null when one time randomization is
not supported.

[Alexei Svitkine (slow), 2012/08/20 15:57:40]

No longer needed since the param is removed.

+             const std::string& name,
              Probability total_probability,
              const std::string& default_group_name);
   virtual ~FieldTrial();
@@ -202,11 +215,9 @@
   // Returns the group_name. A winner need not have been chosen.
   std::string group_name_internal() const { return group_name_; }
 
-  // Calculates a uniformly-distributed double between [0.0, 1.0) given
-  // a |client_id| and a |trial_name| (the latter is used as salt to avoid
-  // separate one-time randomized trials from all having the same results).
-  static double HashClientId(const std::string& client_id,
-                             const std::string& trial_name);
+  // Entropy provider that will be used if the field trial uses one-time
+  // randomization. Weak pointer, owned by FieldTrialList.
+  const EntropyProvider* const entropy_provider_;

[jar (doing other things), 2012/08/17 19:06:19]

Why is this needed on a per-FieldTrial basis?  I would have expected the entropy
to be calculated exactly once. Why do we need this hanging around (resulting in
having to talk about weak pointers)?

Note the original code was a static method... which could equally well have
existed in the TrialList class.

Perhaps this method could even be passed in when needed, closer to
one-time-randomization time.

[Alexei Svitkine (slow), 2012/08/20 15:57:40]

The entropy provider is responsible for providing a random number dice roll
based on the entropy and the trial name. So calculating it exactly once doesn't
work.

We also can't just calculate the double in FieldTrialList and pass to that value
to the constructor because we only want to do this for one time randomized field
trials - and it is not known whether a trial will be such until
|UseOneTimeRandomization()| is called.

 
   // The name of the field trial, as can be found via the FieldTrialList.
   const std::string name_;
@@ -273,16 +284,16 @@
                                             const std::string& group_name) = 0;
 
    protected:
-    virtual ~Observer() {}
+    virtual ~Observer();
   };
 
   // This singleton holds the global list of registered FieldTrials.
   //
-  // |client_id| should be an opaque, diverse ID for this client that does not
-  // change between sessions, to enable one-time randomized trials. The empty
-  // string may be provided, in which case one-time randomized trials will
-  // not be available.
-  explicit FieldTrialList(const std::string& client_id);
+  // To support one-time randomized field trials, specify a non-NULL
+  // |entropy_provider| which should be a source of uniformly distributed
+  // entropy values. Takes ownership of |entropy_provider|.

[jar (doing other things), 2012/08/17 19:06:19]

nit: Comment should be explicit that a NULL value is legal.

[Alexei Svitkine (slow), 2012/08/20 15:57:40]

Done.

+  explicit FieldTrialList(FieldTrial::EntropyProvider* entropy_provider);

[Ilya Sherman, 2012/08/17 17:53:20]

Optional nit: I would pass this in as a scoped_ptr<> as well, and transfer
ownership internally.  The motivation for this pattern is that it's then harder
for the implementation to e.g. return early and fail to take ownership.  That's
obviously not a problem for the current implementation, but explicit transfers
of ownership make it less likely for this sort of error to creep in in the
future.

[Alexei Svitkine (slow), 2012/08/17 17:58:58]

I tried that, but then FieldTrialList(NULL) doesn't compile.

[Ilya Sherman, 2012/08/17 18:16:55]

How about FieldTrialList<scoped_ptr<FieldTrial::EntropyProvider>()>?  If that
doesn't work either, no worries, this is ok as it is.

+
   // Destructor Release()'s references to all registered FieldTrial instances.
   ~FieldTrialList();
 
@@ -393,12 +404,6 @@
   // when constructing the FieldTrialList singleton.
   static bool IsOneTimeRandomizationEnabled();
 
-  // Returns an opaque, diverse ID for this client that does not change
-  // between sessions.
-  //
-  // Returns the empty string if one-time randomization is not enabled.
-  static const std::string& client_id();
-
  private:
   // A map from FieldTrial names to the actual instances.
   typedef std::map<std::string, FieldTrial*> RegistrationList;
@@ -428,9 +433,9 @@
   base::Lock lock_;
   RegistrationList registered_;
 
-  // An opaque, diverse ID for this client that does not change
-  // between sessions, or the empty string if not initialized.
-  std::string client_id_;
+  // Entropy provider to be used for one-time randomized field trials. If NULL,
+  // one-time randomization is not supported.
+  scoped_ptr<FieldTrial::EntropyProvider> entropy_provider_;
 
   // List of observers to be notified when a group is selected for a FieldTrial.
   scoped_refptr<ObserverListThreadSafe<Observer> > observer_list_;