Pulling user experiment code from BrowserDistribution to a new class.

chrome/installer/setup/setup_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 <windows.h>
 #include <msi.h>
 #include <shellapi.h>
 #include <shlobj.h>
 
 #include <string>
@@ skipping 42 matching lines @@
 #include "chrome/installer/util/installation_state.h"
 #include "chrome/installer/util/installation_validator.h"
 #include "chrome/installer/util/installer_state.h"
 #include "chrome/installer/util/l10n_string_util.h"
 #include "chrome/installer/util/logging_installer.h"
 #include "chrome/installer/util/lzma_util.h"
 #include "chrome/installer/util/master_preferences.h"
 #include "chrome/installer/util/master_preferences_constants.h"
 #include "chrome/installer/util/self_cleaning_temp_dir.h"
 #include "chrome/installer/util/shell_util.h"
+#include "chrome/installer/util/user_experiment.h"
 #include "chrome/installer/util/util_constants.h"
 
 #include "installer_util_strings.h"  // NOLINT
 
 using installer::InstallerState;
 using installer::InstallationState;
 using installer::InstallationValidator;
 using installer::Product;
 using installer::ProductState;
 using installer::Products;
@@ skipping 843 matching lines @@
       // If installation failed, use the path to the currently running setup.
       // If installation succeeded, use the path to setup in the installer dir.
       base::FilePath setup_path(cmd_line.GetProgram());
       if (InstallUtil::GetInstallReturnCode(install_status) == 0) {
         setup_path = installer_state.GetInstallerDirectory(*installer_version)
             .Append(setup_path.BaseName());
       }
       for (Products::const_iterator it = products.begin(); it < products.end();
            ++it) {
         const Product& product = **it;
-        product.distribution()->LaunchUserExperiment(setup_path,
+        installer::UserExperiment::LaunchUserExperiment(setup_path,

[grt (UTC plus 2), 2013/02/22 16:47:56]

only call this for Google Chrome (not for Chromium, canary, Chrome Frame, the
binaries, etc...).

[gab, 2013/02/22 19:44:53]

Alternatively we could make LaunchUserExperiment and the likes call into the
current browser distribution to know whether it should run the experiment; given
only the google_chrome_distribution will return true I'm not sure what's
achieved by moving this code out of google_chrome_distribution...

You mentioned bloat, but user_experiment.cc is still in installer_util (or is
the goal to carve it out later?).

Overall I think I prefer having BrowserDistribution::ShouldRunExperimentX() over
having the consumer code (i.e., here) check for #if defined GOOGLE_CHROME_BUILD
and for distribution == CHROME_BROWSER

Maybe even just a generic BrowserDistribution::ShouldRunExperiments() if there
is no need to special case each individual experiments.

[huangs, 2013/02/22 20:32:37]

It seems Canary *does* call the experiment, since the parent class of
GoogleChromeSxSDistribution is GoogleChromeDistribution.

[huangs, 2013/02/22 20:32:37]

I ended up defining

enum UserExperiment::ExperimentCode {...}

use

UserExperiment::IsExperimentSupported((BrowserDistribution::Type,
ExperimentCode)

to do the (currently trivial) decision logic, and let the (3 existing) callers
call this routine to decide.

[gab, 2013/02/25 22:09:53]

I don't like the paradigm:

if (ShouldIDoCrazyThingX())
  DoCrazyThingX();

as it makes room for the caller not knowing or simply forgetting to check the
condition and end up calling DoCrazyThingX() straight up (which assumes the
caller has made the check); how about we put the check at the top of every
experiment method instead, forcing an early return if we shouldn't do the
experiment?

[huangs, 2013/02/26 03:22:25]

If there are expensive initialization needed, they can be placed after if(). 
But they don't in this case, so I'll make the change.

Also, should the name be changed, to reflect that experiment may not be run,
e.g., "LaunchUserExperimentIfRequired()"?  

[gab, 2013/02/26 15:33:34]

Sure.

[huangs, 2013/02/26 16:44:11]

Done (changed to *IfRequired()).

             install_status, *installer_version, product, system_install);
       }
     }
   }
 
   // Delete the master profile file if present. Note that we do not care about
   // rollback here and we schedule for deletion on reboot if the delete fails.
   // As such, we do not use DeleteTreeWorkItem.
   if (cmd_line.HasSwitch(installer::switches::kInstallerData)) {
     base::FilePath prefs_path(cmd_line.GetSwitchValuePath(
@@ skipping 485 matching lines @@
         cmd_line.GetSwitchValueASCII(installer::switches::kExperimentGroup);
     DCHECK_NE(-1, flavor);
     if (flavor == -1) {
       *exit_code = installer::UNKNOWN_STATUS;
     } else {
       const Products& products = installer_state->products();
       for (Products::const_iterator it = products.begin(); it < products.end();
            ++it) {
         const Product& product = **it;
         BrowserDistribution* browser_dist = product.distribution();
-        browser_dist->InactiveUserToastExperiment(
-            flavor, ASCIIToUTF16(experiment_group), product,
+        installer::UserExperiment::InactiveUserToastExperiment(
+            browser_dist, flavor, ASCIIToUTF16(experiment_group), product,
             installer_state->target_path());
       }
     }
   } else if (cmd_line.HasSwitch(installer::switches::kSystemLevelToast)) {
     const Products& products = installer_state->products();
     for (Products::const_iterator it = products.begin(); it < products.end();
          ++it) {
       const Product& product = **it;
       BrowserDistribution* browser_dist = product.distribution();
       // We started as system-level and have been re-launched as user level
       // to continue with the toast experiment.
       Version installed_version;
       InstallUtil::GetChromeVersion(browser_dist, true, &installed_version);
       if (!installed_version.IsValid()) {
         LOG(ERROR) << "No installation of "
                    << browser_dist->GetAppShortCutName()
                    << " found for system-level toast.";
       } else {
-        browser_dist->LaunchUserExperiment(cmd_line.GetProgram(),
-                                           installer::REENTRY_SYS_UPDATE,
-                                           installed_version, product, true);
+        installer::UserExperiment::LaunchUserExperiment(cmd_line.GetProgram(),
+            installer::REENTRY_SYS_UPDATE, installed_version, product, true);
       }
     }
   } else if (cmd_line.HasSwitch(
                  installer::switches::kChromeFrameReadyModeOptIn)) {
     *exit_code = InstallUtil::GetInstallReturnCode(
         installer::ChromeFrameReadyModeOptIn(original_state, *installer_state));
   } else if (cmd_line.HasSwitch(
                  installer::switches::kChromeFrameReadyModeTempOptOut)) {
     *exit_code = InstallUtil::GetInstallReturnCode(
         installer::ChromeFrameReadyModeTempOptOut(original_state,
@@ skipping 284 matching lines @@
   if (!(installer_state.is_msi() && is_uninstall))
     // Note that we allow the status installer::UNINSTALL_REQUIRES_REBOOT
     // to pass through, since this is only returned on uninstall which is
     // never invoked directly by Google Update.
     return_code = InstallUtil::GetInstallReturnCode(install_status);
 
   VLOG(1) << "Installation complete, returning: " << return_code;
 
   return return_code;
 }