Pulling user experiment code from BrowserDistribution to a new class.

chrome/installer/setup/setup_main.cc

Index: chrome/installer/setup/setup_main.cc
diff --git a/chrome/installer/setup/setup_main.cc b/chrome/installer/setup/setup_main.cc
index 12fd98d907ef58013a7723727b809ffd55c2436a..32c1b9ec8097b619ae8d64e7d45e0434d3590a73 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -60,6 +60,7 @@
 #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
@@ -923,7 +924,7 @@ installer::InstallStatus InstallProductsHelper(
       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);
       }
     }
@@ -1429,8 +1430,8 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
            ++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());
       }
     }
@@ -1449,9 +1450,8 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
                    << 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(