Actually update FieldTrialEntry's activated field

base/metrics/field_trial.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.
 
 // FieldTrial is a class for handling details of statistical experiments
 // performed by actual users in the field (i.e., in a shipped or beta product).
 // All code is called exclusively on the UI thread currently.
 //
 // The simplest example is an experiment to see whether one of two options
 // produces "better" results across our user population.  In that scenario, UMA
@@ skipping 248 matching lines @@
   // untouched.
   bool GetActiveGroup(ActiveGroup* active_group) const;
 
   // Returns the trial name and selected group name for this field trial via
   // the output parameter |field_trial_state|, but only if the trial has not
   // been disabled. In that case, true is returned and |field_trial_state| is
   // filled in; otherwise, the result is false and |field_trial_state| is left
   // untouched.
   bool GetState(State* field_trial_state);
 
-  // Adds the field trial to the allocator whose memory is to be shared with
-  // child processes. Assumes the calling code has a lock around the call to
-  // this function, since the check for the allocator is not thread-safe.
-  void AddToAllocatorWhileLocked(
-      base::SharedPersistentMemoryAllocator* allocator);
-
   // Returns the group_name. A winner need not have been chosen.
   std::string group_name_internal() const { return group_name_; }
 
   // The name of the field trial, as can be found via the FieldTrialList.
   const std::string trial_name_;
 
   // The maximum sum of all probabilities supplied, which corresponds to 100%.
   // This is the scaling factor used to adjust supplied probabilities.
   const Probability divisor_;
 
@@ skipping 27 matching lines @@
   // appropriate.
   bool forced_;
 
   // Specifies whether the group choice has been reported to observers.
   bool group_reported_;
 
   // Whether this trial is registered with the global FieldTrialList and thus
   // should notify it when its group is queried.
   bool trial_registered_;
 
+  // Reference to related field trial struct and data in shared memory.
+  SharedPersistentMemoryAllocator::Reference ref_;
+
   // When benchmarking is enabled, field trials all revert to the 'default'
   // group.
   static bool enable_benchmarking_;
 
   DISALLOW_COPY_AND_ASSIGN(FieldTrial);
 };
 
 //------------------------------------------------------------------------------
 // Class with a list of all active field trials.  A trial is active if it has
 // been registered, which includes evaluating its state based on its probaility.
@@ skipping 184 matching lines @@
   // to being part of some specific field_group (and hence the group_name is
   // also finalized for that field_trial).
   static void AddObserver(Observer* observer);
 
   // Remove an observer.
   static void RemoveObserver(Observer* observer);
 
   // Notify all observers that a group has been finalized for |field_trial|.
   static void NotifyFieldTrialGroupSelection(FieldTrial* field_trial);
 
+  // Activate the corresponding field trial entry struct in shared memory.
+  static void ActivateFieldTrialEntry(FieldTrial* field_trial);
+
   // Return the number of active field trials.
   static size_t GetFieldTrialCount();
 
+  // Adds the field trial to the allocator.
+  static void AddToAllocator(FieldTrial* field_trial);

[Alexei Svitkine (slow), 2016/10/25 21:12:29]

I don't think these should be in the public API, since we only want to use them
internally.

So I'd like to see a setup that doesn't expose them.

So, FieldTrialList is a friend of FieldTrial - so FieldTrialList can use private
function in FieldTrial.

So one thing that could be done is having these functions still be members of
FieldTrial and take any necessary parameters that could be passed in or return
values.

For example, AddToAllocator can take the pointer to the lock and to the
allocator - or it can omit the pointer to the lock and require the caller to
hold the lock - which might be reasonable.

ActivateFieldTrialEntry() can take the allocator as well.

[lawrencewu, 2016/10/26 13:43:15]

Hmm, I'm not sure that solves the problem of adding the field trial entry from
FieldTrial::FinalizeGroupChoice, because since it's a method in FieldTrial it
has no global lock to pass in.

We could possibly solve it by making FieldTrial a friend of FieldTrialList, and
then making these two functions private. What do you think?

[Alexei Svitkine (slow), 2016/10/26 14:49:02]

Ah right. Well, ok how about just having:

 OnGroupFinalized(FieldTrial*) in the FieldTrialList that FinalizeGroupChoice()
will call. We still have a public static function, but it's sufficiently narrow
scoped and you can add a comment that it's only meant to be by FieldTrial.

Also the name doesn't suggest to callers that it's something useful for them to
use (e.g. unlike ActivateFieldTrialEntry which might make people thing it does
something the want).

[lawrencewu, 2016/10/26 15:41:38]

Sounds good, done.

+
  private:
   // Allow tests to access our innards for testing purposes.
   FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest, InstantiateAllocator);
   FRIEND_TEST_ALL_PREFIXES(FieldTrialListTest, AddTrialsToAllocator);
 
   // Expects a mapped piece of shared memory |shm| that was created from the
   // browser process's field_trial_allocator and shared via the command line.
   // This function recreates the allocator, iterates through all the field
   // trials in it, and creates them via CreateFieldTrial().
   static void CreateTrialsFromSharedMemory(
@@ skipping 37 matching lines @@
   // Entropy provider to be used for one-time randomized field trials. If NULL,
   // one-time randomization is not supported.
   std::unique_ptr<const FieldTrial::EntropyProvider> entropy_provider_;
 
   // List of observers to be notified when a group is selected for a FieldTrial.
   scoped_refptr<ObserverListThreadSafe<Observer> > observer_list_;
 
   // Allocator used to instantiate field trial in child processes. In the
   // future, we may want to move this to a more generic place if we want to
   // start passing more data other than field trials.
-  std::unique_ptr<base::SharedPersistentMemoryAllocator>
-      field_trial_allocator_ = nullptr;
+  std::unique_ptr<SharedPersistentMemoryAllocator> field_trial_allocator_ =
+      nullptr;
 
 #if defined(OS_WIN)
   // Readonly copy of the handle to the allocator. Needs to be a member variable
   // because it's needed from both CopyFieldTrialStateToFlags() and
   // AppendFieldTrialHandleIfNeeded().
   HANDLE readonly_allocator_handle_ = nullptr;
 #endif
 
   DISALLOW_COPY_AND_ASSIGN(FieldTrialList);
 };
 
 }  // namespace base
 
 #endif  // BASE_METRICS_FIELD_TRIAL_H_