Add one-time randomization support for FieldTrial, and the ability to

chrome/browser/browser_main.cc

 // Copyright (c) 2011 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/browser_main.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 225 matching lines @@
     net::SSLConfigService::EnableDNSCertProvenanceChecking();
   }
 
   if (parsed_command_line().HasSwitch(switches::kEnableTcpFastOpen))
     net::set_tcp_fastopen_enabled(true);
 
   PostEarlyInitialization();
 }
 
 // This will be called after the command-line has been mutated by about:flags
-void BrowserMainParts::SetupFieldTrials() {
-  // Note: make sure to call ConnectionFieldTrial() before
-  // ProxyConnectionsFieldTrial().
-  ConnectionFieldTrial();
-  SocketTimeoutFieldTrial();
-  ProxyConnectionsFieldTrial();
-  prerender::ConfigurePrefetchAndPrerender(parsed_command_line());
-  SpdyFieldTrial();
-  ConnectBackupJobsFieldTrial();
-  SSLFalseStartFieldTrial();
+MetricsService* BrowserMainParts::SetupMetricsAndFieldTrials(
+    const CommandLine& parsed_command_line,
+    PrefService* local_state) {
+  // Must initialize metrics after labs have been converted into switches,
+  // but before field trials are set up (so that client ID is available for
+  // one-time randomized field trials).
+  MetricsService* metrics = InitializeMetrics(parsed_command_line, local_state);
+
+  // Initialize FieldTrialList to support FieldTrials that use one-time
+  // randomization. The client ID will be empty if the user has not opted
+  // to send metrics.
+  field_trial_list_.reset(new base::FieldTrialList(metrics->GetClientId()));
+
+  SetupFieldTrials();
+
+  // Initialize FieldTrialSynchronizer system. This is a singleton and is used
+  // for posting tasks via NewRunnableMethod. Its deleted when it goes out of
+  // scope. Even though NewRunnableMethod does AddRef and Release, the object
+  // will not be deleted after the Task is executed.
+  field_trial_synchronizer_ = new FieldTrialSynchronizer();
+
+  return metrics;
 }
 
 // This is an A/B test for the maximum number of persistent connections per
 // host. Currently Chrome, Firefox, and IE8 have this value set at 6. Safari
 // uses 4, and Fasterfox (a plugin for Firefox that supposedly configures it to
 // run faster) uses 8. We would like to see how much of an effect this value has
 // on browsing. Too large a value might cause us to run into SYN flood detection
 // mechanisms.
 void BrowserMainParts::ConnectionFieldTrial() {
   const base::FieldTrial::Probability kConnectDivisor = 100;
@@ skipping 262 matching lines @@
 void BrowserMainParts::InitializeMainThread() {
   const char* kThreadName = "CrBrowserMain";
   base::PlatformThread::SetName(kThreadName);
   main_message_loop().set_thread_name(kThreadName);
 
   // Register the main thread by instantiating it, but don't call any methods.
   main_thread_.reset(new BrowserThread(BrowserThread::UI,
                                        MessageLoop::current()));
 }
 
+// BrowserMainParts: |SetupMetricsAndFieldTrials()| related --------------------
+
+// Initializes the metrics service with the configuration for this process,
+// returning the created service (guaranteed non-NULL).
+MetricsService* BrowserMainParts::InitializeMetrics(
+    const CommandLine& parsed_command_line,
+    const PrefService* local_state) {
+#if defined(OS_WIN)
+  if (parsed_command_line.HasSwitch(switches::kChromeFrame))
+    MetricsLog::set_version_extension("-F");
+#elif defined(ARCH_CPU_64_BITS)
+  MetricsLog::set_version_extension("-64");
+#endif  // defined(OS_WIN)
+
+  MetricsService* metrics = g_browser_process->metrics_service();
+
+  if (parsed_command_line.HasSwitch(switches::kMetricsRecordingOnly) ||
+      parsed_command_line.HasSwitch(switches::kEnableBenchmarking)) {
+    // If we're testing then we don't care what the user preference is, we turn
+    // on recording, but not reporting, otherwise tests fail.
+    metrics->StartRecordingOnly();
+  } else {

[jar (doing other things), 2011/05/03 00:17:47]

Please use an early return within the if block, and avoid the else block with
its indentation.

I know you just moved this code... but you may as well clean up the readability
during the relocation.

[Jói, 2011/05/03 17:41:47]

Done.

+    // If the user permits metrics reporting with the checkbox in the
+    // prefs, we turn on recording.  We disable metrics completely for
+    // non-official builds.
+#if defined(GOOGLE_CHROME_BUILD)

[jar (doing other things), 2011/05/03 00:17:47]

I'm not sure if an early return works as well with a #if, but these nested
returns are ugly and hard to read.  I'd suggest:
#if !defined(GOOGLE_CHROME_BUILD)
        return metrics;
#endif  // GOOGLE_CHROME_METRICS

I'm not sure if you'd have to use a #else clause, but if you don't it would be a
win to not have to read these nested preprocessor conditionals.

[Jói, 2011/05/03 17:41:47]

After removing the else (previous comment) it seems cleanest to just keep the
return at end.  I added a comment on the #endif at line 588 to make it easy to
see where the whole #ifdef block ends.

+#if defined(OS_CHROMEOS)
+    bool enabled = chromeos::MetricsCrosSettingsProvider::GetMetricsStatus();
+#else
+    bool enabled = local_state->GetBoolean(prefs::kMetricsReportingEnabled);
+#endif  // #if defined(OS_CHROMEOS)
+    if (enabled) {
+      metrics->Start();
+      chrome_browser_net_websocket_experiment::
+          WebSocketExperimentRunner::Start();
+    }
+#endif
+  }
+
+  return metrics;
+}
+
+void BrowserMainParts::SetupFieldTrials() {
+  // Note: make sure to call ConnectionFieldTrial() before
+  // ProxyConnectionsFieldTrial().
+  ConnectionFieldTrial();
+  SocketTimeoutFieldTrial();
+  ProxyConnectionsFieldTrial();
+  prerender::ConfigurePrefetchAndPrerender(parsed_command_line());
+  SpdyFieldTrial();
+  ConnectBackupJobsFieldTrial();
+  SSLFalseStartFieldTrial();
+}
+
 // -----------------------------------------------------------------------------
 // TODO(viettrungluu): move more/rest of BrowserMain() into above structure
 
 namespace {
 
 // This function provides some ways to test crash and assertion handling
 // behavior of the program.
 void HandleTestParameters(const CommandLine& command_line) {
   // This parameter causes an assertion.
   if (command_line.HasSwitch(switches::kBrowserAssertTest)) {
@@ skipping 208 matching lines @@
       // Precreate the desktop and window station used by the renderers.
       sandbox::TargetPolicy* policy = broker_services->CreatePolicy();
       sandbox::ResultCode result = policy->CreateAlternateDesktop(use_winsta);
       CHECK(sandbox::SBOX_ERROR_FAILED_TO_SWITCH_BACK_WINSTATION != result);
       policy->Release();
     }
   }
 #endif
 }
 
-// Initializes the metrics service with the configuration for this process,
-// returning the created service (guaranteed non-NULL).
-MetricsService* InitializeMetrics(const CommandLine& parsed_command_line,
-                                  const PrefService* local_state) {
-#if defined(OS_WIN)
-  if (parsed_command_line.HasSwitch(switches::kChromeFrame))
-    MetricsLog::set_version_extension("-F");
-#elif defined(ARCH_CPU_64_BITS)
-  MetricsLog::set_version_extension("-64");
-#endif  // defined(OS_WIN)
-
-  MetricsService* metrics = g_browser_process->metrics_service();
-
-  if (parsed_command_line.HasSwitch(switches::kMetricsRecordingOnly) ||
-      parsed_command_line.HasSwitch(switches::kEnableBenchmarking)) {
-    // If we're testing then we don't care what the user preference is, we turn
-    // on recording, but not reporting, otherwise tests fail.
-    metrics->StartRecordingOnly();
-  } else {
-    // If the user permits metrics reporting with the checkbox in the
-    // prefs, we turn on recording.  We disable metrics completely for
-    // non-official builds.
-#if defined(GOOGLE_CHROME_BUILD)
-#if defined(OS_CHROMEOS)
-    bool enabled = chromeos::MetricsCrosSettingsProvider::GetMetricsStatus();
-#else
-    bool enabled = local_state->GetBoolean(prefs::kMetricsReportingEnabled);
-#endif  // #if defined(OS_CHROMEOS)
-    if (enabled) {
-      metrics->Start();
-      chrome_browser_net_websocket_experiment::
-          WebSocketExperimentRunner::Start();
-    }
-#endif
-  }
-
-  return metrics;
-}
-
 // Initializes the profile, possibly doing some user prompting to pick a
 // fallback profile. Returns the newly created profile, or NULL if startup
 // should not continue.
 Profile* CreateProfile(const MainFunctionParams& parameters,
                        const FilePath& user_data_dir) {
   const CommandLine& browser_command_line = *CommandLine::ForCurrentProcess();
   Profile* profile = browser_command_line.HasSwitch(switches::kMultiProfiles) ?
       g_browser_process->profile_manager()->GetLastUsedProfile(user_data_dir) :
       g_browser_process->profile_manager()->GetDefaultProfile(user_data_dir);
   if (profile)
@@ skipping 505 matching lines @@
 
   // Initialize thread watcher system. This is a singleton and is used by
   // WatchDogThread to keep track of information about threads that are being
   // watched.
   scoped_ptr<ThreadWatcherList> thread_watcher_list(new ThreadWatcherList());
 
   // Convert active labs into switches. Modifies the current command line.
   about_flags::ConvertFlagsToSwitches(local_state,
                                       CommandLine::ForCurrentProcess());
 
-  // Now the command line has been mutated based on about:flags, we can run some
-  // field trials
-  parts->SetupFieldTrials();
-
-  // Initialize FieldTrialSynchronizer system. This is a singleton and is used
-  // for posting tasks via NewRunnableMethod. Its deleted when it goes out of
-  // scope. Even though NewRunnableMethod does AddRef and Release, the object
-  // will not be deleted after the Task is executed.
-  scoped_refptr<FieldTrialSynchronizer> field_trial_synchronizer(
-      new FieldTrialSynchronizer());
+  // Now the command line has been mutated based on about:flags, we can
+  // set up metrics and initialize field trials.
+  MetricsService* metrics = parts->SetupMetricsAndFieldTrials(
+      parsed_command_line, local_state);
 
   // Now that all preferences have been registered, set the install date
   // for the uninstall metrics if this is our first run. This only actually
   // gets used if the user has metrics reporting enabled at uninstall time.
   int64 install_date =
       local_state->GetInt64(prefs::kUninstallMetricsInstallDate);
   if (install_date == 0) {
     local_state->SetInt64(prefs::kUninstallMetricsInstallDate,
                           base::Time::Now().ToTimeT());
   }
@@ skipping 323 matching lines @@
     sdch_trial->AppendGroup("global_disable_sdch",
                             kSDCH_DISABLE_PROBABILITY);
     if (sdch_enabled != sdch_trial->group())
       sdch_supported_domain = "never_enabled_sdch_for_any_domain";
   }
 
   net::SdchManager sdch_manager;  // Singleton database.
   sdch_manager.set_sdch_fetcher(new SdchDictionaryFetcher);
   sdch_manager.EnableSdchSupport(sdch_supported_domain);
 
-  MetricsService* metrics = InitializeMetrics(parsed_command_line, local_state);
   InstallJankometer(parsed_command_line);
 
 #if defined(OS_WIN) && !defined(GOOGLE_CHROME_BUILD)
   if (parsed_command_line.HasSwitch(switches::kDebugPrint)) {
     FilePath path =
         parsed_command_line.GetSwitchValuePath(switches::kDebugPrint);
     printing::PrintedDocument::set_debug_dump_path(path);
   }
 #endif
 
@@ skipping 204 matching lines @@
 #if defined(OS_CHROMEOS)
   // To be precise, logout (browser shutdown) is not yet done, but the
   // remaining work is negligible, hence we say LogoutDone here.
   chromeos::BootTimesLoader::Get()->AddLogoutTimeMarker("LogoutDone",
                                                         false);
   chromeos::BootTimesLoader::Get()->WriteLogoutTimes();
 #endif
   TRACE_EVENT_END("BrowserMain", 0, 0);
   return result_code;
 }