Registering field trial for a feature overridden in chrome://flags.

components/flags_ui/flags_state.cc

 // Copyright 2015 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.
 
 #include "components/flags_ui/flags_state.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/callback.h"
@@ skipping 179 matching lines @@
     result->Append(std::move(value));
   }
   return result;
 }
 
 // Registers variation parameters specified by |feature_variation| for the field
 // trial named |feature_trial_name|, unless a group for this trial has already
 // been created (e.g. via command-line switches that take precedence over
 // about:flags). In the trial, the function creates a new constant group called
 // |kTrialGroupAboutFlags|.
-void RegisterFeatureVariationParameters(
+base::FieldTrial* RegisterFeatureVariationParameters(
     const std::string& feature_trial_name,
     const FeatureEntry::FeatureVariation& feature_variation) {
   std::map<std::string, std::string> params;
   for (int i = 0; i < feature_variation.num_params; ++i) {
     params[feature_variation.params[i].param_name] =
         feature_variation.params[i].param_value;
   }
 
   bool success = variations::AssociateVariationParams(
       feature_trial_name, internal::kTrialGroupAboutFlags, params);
-  if (success) {
-    // Successful association also means that no group is created and selected
-    // for the trial, yet. Thus, create the trial to select the group. This way,
-    // the parameters cannot get overwritten in later phases (such as from the
-    // server).
-    base::FieldTrial* trial = base::FieldTrialList::CreateFieldTrial(
-        feature_trial_name, internal::kTrialGroupAboutFlags);
-    if (!trial) {
-      DLOG(WARNING) << "Could not create the trial " << feature_trial_name
-                    << " with group " << internal::kTrialGroupAboutFlags;
-    }
+  if (!success) return nullptr;

[Alexei Svitkine (slow), 2016/07/07 18:36:34]

Nit: Separate line please. Chrome C++ code prefers that style generally, so
follow the convention. :)

[jkrcal, 2016/07/11 09:53:12]

Done.

+  // Successful association also means that no group is created and selected
+  // for the trial, yet. Thus, create the trial to select the group. This way,
+  // the parameters cannot get overwritten in later phases (such as from the
+  // server).
+  base::FieldTrial* trial = base::FieldTrialList::CreateFieldTrial(
+      feature_trial_name, internal::kTrialGroupAboutFlags);
+  if (!trial) {
+    DLOG(WARNING) << "Could not create the trial " << feature_trial_name
+                  << " with group " << internal::kTrialGroupAboutFlags;
   }
+  return trial;
 }
 
 }  // namespace
 
 // Keeps track of affected switches for each FeatureEntry, based on which
 // choice is selected for it.
 struct SwitchEntry {
   // Corresponding base::Feature to toggle.
   std::string feature_name;
 
@@ skipping 193 matching lines @@
   flags_storage->SetFlags(no_entries);
 }
 
 void FlagsState::Reset() {
   needs_restart_ = false;
   flags_switches_.clear();
   appended_switches_.clear();
 }
 
 void FlagsState::RegisterAllFeatureVariationParameters(
-    FlagsStorage* flags_storage) {
+    FlagsStorage* flags_storage,
+    base::FeatureList* feature_list) {
   std::set<std::string> enabled_entries;
   GetSanitizedEnabledFlagsForCurrentPlatform(flags_storage, &enabled_entries);
 
   for (size_t i = 0; i < num_feature_entries_; ++i) {
     const FeatureEntry& e = feature_entries_[i];
     if (e.type == FeatureEntry::FEATURE_WITH_VARIATIONS_VALUE) {
       for (int j = 0; j < e.num_options; ++j) {
         const FeatureEntry::FeatureVariation* variation =
             e.VariationForOption(j);
         if (variation != nullptr && enabled_entries.count(e.NameForOption(j))) {
           // If the option is selected by the user & has variation, register it.
-          RegisterFeatureVariationParameters(e.feature_trial_name, *variation);
-          // TODO(jkrcal) The code does not associate the feature with the field
-          // trial |e.feature_trial_name|. The reason is that features
-          // overridden in chrome://flags are translated to command-line flags
-          // and thus treated earlier in the initialization. The fix requires
-          // larger changes. As a result:
-          //  - the API calls to variations::GetVariationParamValueByFeature and
-          //    to variations::GetVariationParamsByFeature do not work; and
-          //  - the API call to base::FeatureList::IsEnabled does not mark the
-          //    field trial as active (and the trial does not appear in UMA).
-          // If the code calls variations::GetVariationParamValue or
-          // variations::GetVariationParams providing the trial name, everything
-          // should work fine.
+          base::FieldTrial* field_trial = RegisterFeatureVariationParameters(
+              e.feature_trial_name, *variation);
+
+          if (!field_trial) continue;

[Alexei Svitkine (slow), 2016/07/07 18:36:34]

Nit: Separate line.

[jkrcal, 2016/07/11 09:53:12]

Done.

+          feature_list->RegisterFieldTrialOverride(
+              e.feature->name,
+              base::FeatureList::OverrideState::OVERRIDE_ENABLE_FEATURE,
+              field_trial);
         }
       }
     }
   }
 }
 
 void FlagsState::GetFlagFeatureEntries(
     FlagsStorage* flags_storage,
     FlagAccess access,
     base::ListValue* supported_entries,
@@ skipping 248 matching lines @@
   }
 
   std::set<std::string> new_enabled_entries =
       base::STLSetIntersection<std::set<std::string>>(platform_entries,
                                                       *result);
 
   result->swap(new_enabled_entries);
 }
 
 }  // namespace flags_ui