Add other variations (with ids) for NTPSnippets to chrome://flags

chrome/browser/chrome_browser_main.cc

Index: chrome/browser/chrome_browser_main.cc
diff --git a/chrome/browser/chrome_browser_main.cc b/chrome/browser/chrome_browser_main.cc
index 1166a7f31c04fbf2c8ea7305a92023145c11118f..51e8463e0fa7b22ec164b80c8b92faec628c5558 100644
--- a/chrome/browser/chrome_browser_main.cc
+++ b/chrome/browser/chrome_browser_main.cc
@@ -730,26 +730,38 @@ void ChromeBrowserMainParts::SetupMetricsAndFieldTrials() {
                   << " list specified.";
   }
 
+  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+
+  // Associate parameters chosen in about:flags and create trial/group for them.
+  flags_ui::PrefServiceFlagsStorage flags_storage(
+      g_browser_process->local_state());
+  std::string variation_ids =
+      about_flags::RegisterAllFeatureVariationParameters(
+          &flags_storage, feature_list.get());
+
   if (command_line->HasSwitch(switches::kForceVariationIds)) {

[Lei Zhang, 2016/07/25 17:37:20]

The command line can have the switch but its value could be an empty string as
well. i.e. users can pass in --foo, when they should be passing in
--foo=foo_val.

[jkrcal, 2016/07/26 09:00:21]

Thanks for pointing out! Such a case in handled (as an erroneous input) in
VariationsHttpHeaderProvider::SetDefaultVariationIds().

+    // Combine the variation ids from chrome://flags with the once from the
+    // command-line flag.
+    if (!variation_ids.empty()) {
+      variation_ids +=
+          "," + command_line->GetSwitchValueASCII(switches::kForceVariationIds);
+    } else {
+      variation_ids =
+          command_line->GetSwitchValueASCII(switches::kForceVariationIds);
+    }
+  }

[Alexei Svitkine (slow), 2016/07/25 19:15:50]

Instead of all this extra work in chrome_browser_main, I would instead prefer
passing the extra ids to the SetDefaultVariationIds() API and doing the logic
there. Also, I think you can just return the extra ids as a std::vector from
RegisterAllFeatureVariationParameters() instead of converting to intermediate
string form.

[jkrcal, 2016/07/26 09:00:21]

I tried to simplify it. Do not like the idea of moving even more into
SetDefaultVariationIds() but feel free to insist :)

+
+  if (!variation_ids.empty()) {
     // Create default variation ids which will always be included in the
     // X-Client-Data request header.
     variations::VariationsHttpHeaderProvider* provider =
         variations::VariationsHttpHeaderProvider::GetInstance();
-    bool result = provider->SetDefaultVariationIds(
-        command_line->GetSwitchValueASCII(switches::kForceVariationIds));
-    CHECK(result) << "Invalid --" << switches::kForceVariationIds
-                  << " list specified.";
+    bool result = provider->SetDefaultVariationIds(variation_ids);
+    CHECK(result) << "Invalid list of variation ids specified (either in --"
+                  << switches::kForceVariationIds << " or in chrome://flags)";
     metrics->AddSyntheticTrialObserver(provider);
   }
 
-  std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
-
-  // Associate parameters chosen in about:flags and create trial/group for them.
-  flags_ui::PrefServiceFlagsStorage flags_storage(
-      g_browser_process->local_state());
-  about_flags::RegisterAllFeatureVariationParameters(&flags_storage,
-    feature_list.get());
-
   feature_list->InitializeFromCommandLine(
       command_line->GetSwitchValueASCII(switches::kEnableFeatures),
       command_line->GetSwitchValueASCII(switches::kDisableFeatures));