Activate prepopulated fieldtrials

chrome/browser/chrome_browser_main.cc

 // 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.
 
 #include "chrome/browser/chrome_browser_main.h"
 
 #include <set>
 #include <string>
 #include <vector>
 
@@ skipping 615 matching lines @@
 
     // Create field trials without activating them, so that this behaves in a
     // consistent manner with field trials created from the server.
     bool result = base::FieldTrialList::CreateTrialsFromString(
         command_line->GetSwitchValueASCII(switches::kForceFieldTrials),
         base::FieldTrialList::DONT_ACTIVATE_TRIALS,
         unforceable_field_trials);
     CHECK(result) << "Invalid --" << switches::kForceFieldTrials
                   << " list specified.";
   }
+
+#if defined(GOOGLE_CHROME_BUILD) && defined(OFFICIAL_BUILD)

[Alexei Svitkine (slow), 2015/07/10 19:22:30]

I think OFFICIAL_BUILD is not needed here. I think that macro is not meant to
change behavior - only control some optimization settings for the build. (Though
there may be existing code that does it wrong).

[danduong, 2015/07/10 19:36:56]

Hmm... If we were to do this, would I still be able to get field trial config
enabled by default for developers testing Google branded features?

[brettw, 2015/07/10 19:41:21]

Yeah, I always get these confused but "chrome_build" is what I think we want.

[Alexei Svitkine (slow), 2015/07/10 19:44:41]

Interesting. I had assumed that's not really a use case - but interested in
understanding the exact use case you have in mind.

If we do want to support this, we should disable VariationsService created
trials, if the config is used instead. (In fact, perhaps this code should live
inside VariationsService instead of here?)

[danduong, 2015/07/10 23:02:36]

I guess that would be a lesser use-case. For simplicity, I switched to if
!defined(GOOGLE_CHROME_BUILD)

+  std::string enable_field_trial_testing = command_line->GetSwitchValueASCII(
+      switches::kEnableFieldTrialTestingConfig);

[Alexei Svitkine (slow), 2015/07/10 19:22:29]

Convention for switches that start with "enable_*" is just to test for their
presence.

Since you actually want a disable switch, let's follow the convention for that.

Call the switch kDisableFieldTrialTestingConfig and then simplify the code below
to just be

if (!command_line->HasSwitch(kDisable..) && !command_line->HasSwitch(kForce...))
{
  ...
}

I think the other thing this should be checking for is if the user specified a
custom variations-server-url - since that actually turns on variations service
for unofficial builds - and in that case, we shouldn't use the default config
either.

[danduong, 2015/07/10 19:36:56]

I can add a check for the variations server stuff.

I'm a little concerned by the loss of flexibility if I switch it to purely a
disable flag. I know laforge was interested in an easy way for the test team to
enable this configuration on certain builds and it'd be nice to simplify the
configuration of the perf bot builds (add 1 flag versus several flags that we
currently have).

[Alexei Svitkine (slow), 2015/07/10 19:44:41]

Right... I remember now - because the perf bots actually do run Chrome builds
rather than Chromium? In that case, it makes sense to have available for those
builds. But we should figure out what we want to do about UMA reporting.
--force-fieldtrials currently disables UMA. Should we do likewise if the config
is used? Otherwise, if people end up using this in the wild, they may affect our
percentages.

In fact, we probably should have a histogram reporting that this was used, so we
can monitor if this starts getting used in the wild. (e.g. malware may set this
to force a "known" configuration?)

Sigh... the more I think about this, the less I like having official builds
support this. Perhaps we can just keep the existing perfbot set up for official
builds?

[danduong, 2015/07/10 23:02:35]

Yup. Let's simplify with just using a disable flag.

+  if (enable_field_trial_testing.empty()) {

[brettw, 2015/07/10 19:41:21]

No {} one one-line conditionals for consistency.

[danduong, 2015/07/10 23:02:36]

Removed this piece of code altogether.

+    enable_field_trial_testing = "1";
+  }
+
+  if (enable_field_trial_testing == "1" &&
+      !command_line->HasSwitch(switches::kForceFieldTrials)) {
+    bool result = chrome_variations::AssociateDefaultFieldTrialConfig();
+    CHECK(result) << " Enabling field trial testing configuration failed.";
+  }
+#endif  // defined(GOOGLE_CHROME_BUILD) && defined(OFFICIAL_BUILD)
+
   if (command_line->HasSwitch(switches::kForceVariationIds)) {
     // 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.";
     metrics->AddSyntheticTrialObserver(provider);
@@ skipping 1134 matching lines @@
   chromeos::CrosSettings::Shutdown();
 #endif  // defined(OS_CHROMEOS)
 #endif  // defined(OS_ANDROID)
 }
 
 // Public members:
 
 void ChromeBrowserMainParts::AddParts(ChromeBrowserMainExtraParts* parts) {
   chrome_extra_parts_.push_back(parts);
 }