Pulling user experiment code from BrowserDistribution to a new class.

chrome/installer/util/product.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/installer/util/product.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/process_util.h"
 #include "base/win/registry.h"
 #include "chrome/installer/util/chrome_app_host_operations.h"
 #include "chrome/installer/util/chrome_binaries_operations.h"
 #include "chrome/installer/util/chrome_browser_operations.h"
 #include "chrome/installer/util/chrome_browser_sxs_operations.h"
 #include "chrome/installer/util/chrome_frame_operations.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/install_util.h"
 #include "chrome/installer/util/master_preferences.h"
 #include "chrome/installer/util/master_preferences_constants.h"
 #include "chrome/installer/util/product_operations.h"
+#include "chrome/installer/util/user_experiment.h"
 
 using base::win::RegKey;
 using installer::MasterPreferences;
 
 namespace installer {
 
 Product::Product(BrowserDistribution* distribution)
     : distribution_(distribution) {
   switch (distribution->GetType()) {
     case BrowserDistribution::CHROME_BROWSER:
@@ skipping 122 matching lines @@
   return operations_->SetChannelFlags(options_, set, channel_info);
 }
 
 void Product::AddDefaultShortcutProperties(
     const base::FilePath& target_exe,
     ShellUtil::ShortcutProperties* properties) const {
   return operations_->AddDefaultShortcutProperties(
       distribution_, target_exe, properties);
 }
 
+void Product::LaunchUserExperiment(const base::FilePath& setup_path,
+                                   InstallStatus status,
+                                   bool system_level) const {
+  if (operations_->ShouldLaunchUserExperiment()) {

[gab, 2013/03/04 23:40:30]

I'm really not a fan of having these two entry points into
LaunchUserExperiments(), only one of which (this one, not the direct call)
checks ShouldLaunchUserExperiment().

I feel having the ShouldLaunchUserExperiment() check at the top of the
experiment methods was better than this, I'll let grt chime in since he's the
one who objected to having the policy at the top of the impl.

[grt (UTC plus 2), 2013/03/05 14:33:22]

what i had in mind is:
  if (distribution_->HasUserExperiments())
    operations_->LaunchUserExperiment(...)
this puts the policy in BrowserDistribution (where all distribution-specific
policy belongs, imo) and the implementation in ProductOperations. decoupling the
two means that future changes in policy don't need to touch the implementation.
also, the implementation can conceivably be tested without having to fake the
correct policy.

[gab, 2013/03/05 14:58:00]

Ah ok, the downside is that callers have to remember to check the policy, but I
agree that being able to test without faking the policy and being able to tweak
the policy without changing the impl is nice (although you get the latter if the
policy is just a method call at the top of the impl).

[huangs, 2013/03/05 15:38:00]

Personally I'd prefer NOT changing Operations, since this just means we'll have
a lot of stub implemenetations.  Also, installer::LaunchUserExperiment() takes a
Products as parameter.  Making ChromeBrowserOperations aware of Products seems
wrong.

[grt (UTC plus 2), 2013/03/05 16:30:49]

If you want to avoid creating stub implementations, then make an abstract
ProductOperationsImpl or somesuch that noops all methods and have the concrete
classes derive from it and override only those that they care about.
Then something else needs to be done. The policy should not be in
ProductOperations as you have it right now. If the current experiment code only
ever applies to the browser, then perhaps it can be refactored so that a Product
isn't passed down into the ProductOperations. Otherwise, look at why Product is
passed down and work out a way to avoid that.

[huangs, 2013/03/05 16:48:34]

Okay, I'm for making ProductOperationsImpl!  Let's do this as a prerequesite
cleanup for this CL.

installer::LauncherUserExperiment() uses the following parts of Product:

AppendProductFlags()  =>  can get from ProductOperations.
distribution()->GetAppShortCutName()
GetUserDataPaths()  =>  Part of Product(), but can be directly injected.

I'll have to think some more.  Meanwhile, let's add ProductOperationImpl first.

+    installer::user_experiment::LaunchUserExperiment(setup_path, status,
+        *this, system_level);
+  }
+}
+
 }  // namespace installer