More installer refactoring in the interest of fixing some bugs and cleaning t...

chrome/installer/util/installation_state.cc

Index: chrome/installer/util/installation_state.cc
===================================================================
--- chrome/installer/util/installation_state.cc	(revision 71802)
+++ chrome/installer/util/installation_state.cc	(working copy)
@@ -9,16 +9,54 @@
 #include "base/version.h"
 #include "base/win/registry.h"
 #include "chrome/installer/util/google_update_constants.h"
-#include "chrome/installer/util/package_properties.h"
 
+namespace {
+
+CommandLine MakeUninstallCommand(const std::wstring& setup_path,
+                                 const std::wstring& uninstall_arguments) {

[robertshield, 2011/01/21 16:58:56]

This looks non-trivial enough to warrant a small unit test.

[grt (UTC plus 2), 2011/01/24 16:07:02]

Done.

+  bool no_program = setup_path.empty();
+
+  // Return a bunch of nothingness.
+  if (no_program && uninstall_arguments.empty())
+    return CommandLine(CommandLine::NO_PROGRAM);
+
+  // Form a full command line string.
+  std::wstring command;
+  command.append(1, L'"')
+      .append(no_program ? L"foo.exe" : setup_path)
+      .append(L"\" ")
+      .append(uninstall_arguments);
+
+  // If we have a program name, return this complete command line.
+  if (!no_program)
+    return CommandLine::FromString(command);
+
+  // Otherwise, make a new instance with the arguments but no program.
+  CommandLine result(CommandLine::NO_PROGRAM);
+  result.AppendArguments(CommandLine::FromString(command), false);
+  return result;
+}
+
+}  // namespace
+
 namespace installer {
 
-ProductState::ProductState() {
+ProductState::ProductState()
+    : uninstall_command_(CommandLine::NO_PROGRAM),
+      msi_(false),
+      multi_install_(false) {
 }
 
-void ProductState::Initialize(bool system_install,
-                              const std::wstring& version_key,
-                              const std::wstring& state_key) {
+bool ProductState::Initialize(bool system_install,
+                              BrowserDistribution::Type type) {
+  return Initialize(system_install,
+                    BrowserDistribution::GetSpecificDistribution(type));
+}
+
+bool ProductState::Initialize(bool system_install,
+                              BrowserDistribution* distribution) {

[robertshield, 2011/01/21 16:58:56]

I'd feel better if this had a test too. The test could hand it a mock browser
distribution that lied about the state and version keys.

[grt (UTC plus 2), 2011/01/24 16:07:02]

Done; see product_state_unittest.cc.

+  const std::wstring version_key(distribution->GetVersionKey());
+  const std::wstring state_key(distribution->GetStateKey());
   const HKEY root_key = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
   base::win::RegKey key(root_key, version_key.c_str(), KEY_QUERY_VALUE);
   std::wstring version_str;
@@ -26,29 +64,72 @@
       == ERROR_SUCCESS) {
     version_.reset(Version::GetVersionFromString(WideToASCII(version_str)));
     if (version_.get() != NULL) {
-      // The product is installed.  Check for the channel value (absent if not
-      // installed/managed by Google Update).
-      if ((key.Open(root_key, state_key.c_str(), KEY_QUERY_VALUE) !=
-          ERROR_SUCCESS) || !channel_.Initialize(key)) {
-        channel_.set_value(std::wstring());
+      // The product is installed.
+      if (key.ReadValue(google_update::kRegOldVersionField, &version_str)
+          == ERROR_SUCCESS) {
+        old_version_.reset(
+            Version::GetVersionFromString(WideToASCII(version_str)));
+      } else {
+        old_version_.reset();
       }
+      if (key.ReadValue(google_update::kRegRenameCmdField, &rename_cmd_)
+          != ERROR_SUCCESS)

[robertshield, 2011/01/21 16:58:56]

NIT WARNING: I find the following more readable:

if (key.ReadValue(google_update::kRegRenameCmdField,
                  &rename_cmd_) != ERROR_SUCCESS)
  rename_cmd.clear();

Also, https://sites.google.com/a/chromium.org/dev/developers/coding-style states
that lines should not begin with operators.

The same applies to a the other places in this method.

[grt (UTC plus 2), 2011/01/24 16:07:02]

I like that better, too.  Thanks.

+        rename_cmd_.clear();
+      // Read from the ClientState key.
+      channel_.set_value(std::wstring());
+      uninstall_command_ = CommandLine(CommandLine::NO_PROGRAM);
+      msi_ = false;
+      multi_install_ = false;
+      if (key.Open(root_key, state_key.c_str(), KEY_QUERY_VALUE)
+          == ERROR_SUCCESS) {
+        std::wstring setup_path;
+        std::wstring uninstall_arguments;
+        // "ap" will be absent if not managed by Google Update.
+        channel_.Initialize(key);
+        // "UninstallString" will be absent for the multi-installer package.
+        key.ReadValue(kUninstallStringField, &setup_path);
+        // "UninstallArguments" will be absent for the multi-installer package.
+        key.ReadValue(kUninstallArgumentsField, &uninstall_arguments);
+        uninstall_command_ =
+            MakeUninstallCommand(setup_path, uninstall_arguments);
+        // "msi" may be absent, 0 or 1
+        DWORD dw_value = 0;
+        msi_ = (key.ReadValueDW(google_update::kRegMSIField, &dw_value)
+                == ERROR_SUCCESS) && (dw_value != 0);
+        // Multi-install is implied or is derived from the command-line.
+        if (distribution->GetType() == BrowserDistribution::CHROME_BINARIES)
+          multi_install_ = true;
+        else
+          multi_install_ = uninstall_command_.HasSwitch(
+              switches::kMultiInstall);

[tommi (sloooow) - chröme, 2011/01/21 21:45:17]

braces

[grt (UTC plus 2), 2011/01/24 16:07:02]

Done.

+      }
     }
   } else {
     version_.reset();
   }
+  return version_.get() != NULL;
 }
 
+FilePath ProductState::GetSetupPath() const {
+  return uninstall_command_.GetProgram();
+}
+
 const Version& ProductState::version() const {
   DCHECK(version_.get() != NULL);
   return *version_;
 }
 
-void ProductState::CopyFrom(const ProductState& other) {
+ProductState& ProductState::CopyFrom(const ProductState& other) {
   channel_.set_value(other.channel_.value());
-  if (other.version_.get() == NULL)
-    version_.reset();
-  else
-    version_.reset(other.version_->Clone());
+  version_.reset(other.version_.get() == NULL ? NULL : other.version_->Clone());
+  old_version_.reset(
+      other.old_version_.get() == NULL ? NULL : other.old_version_->Clone());
+  rename_cmd_ = other.rename_cmd_;
+  uninstall_command_ = other.uninstall_command_;
+  msi_ = other.msi_;
+  multi_install_ = other.multi_install_;
+
+  return *this;
 }
 
 InstallationState::InstallationState() {
@@ -60,55 +141,33 @@
                  unexpected_chrome_browser_distribution_value_);
   COMPILE_ASSERT(BrowserDistribution::CHROME_FRAME == CHROME_FRAME_INDEX,
                  unexpected_chrome_frame_distribution_value_);
+  COMPILE_ASSERT(BrowserDistribution::CHROME_BINARIES == CHROME_BINARIES_INDEX,
+                 unexpected_chrome_frame_distribution_value_);
   DCHECK(type == BrowserDistribution::CHROME_BROWSER ||
-         type == BrowserDistribution::CHROME_FRAME);
+         type == BrowserDistribution::CHROME_FRAME ||
+         type == BrowserDistribution::CHROME_BINARIES);
   return type;
 }
 
-void InstallationState::Initialize(const MasterPreferences& prefs) {
+void InstallationState::Initialize() {
   BrowserDistribution* distribution;
 
   distribution = BrowserDistribution::GetSpecificDistribution(
-      BrowserDistribution::CHROME_BROWSER, prefs);
-  InitializeProduct(false, distribution, &user_products_[CHROME_BROWSER_INDEX]);
-  InitializeProduct(true, distribution,
-      &system_products_[CHROME_BROWSER_INDEX]);
+      BrowserDistribution::CHROME_BROWSER);
+  user_products_[CHROME_BROWSER_INDEX].Initialize(false, distribution);
+  system_products_[CHROME_BROWSER_INDEX].Initialize(true, distribution);
 
   distribution = BrowserDistribution::GetSpecificDistribution(
-      BrowserDistribution::CHROME_FRAME, prefs);
-  InitializeProduct(false, distribution, &user_products_[CHROME_FRAME_INDEX]);
-  InitializeProduct(true, distribution, &system_products_[CHROME_FRAME_INDEX]);
+      BrowserDistribution::CHROME_FRAME);
+  user_products_[CHROME_FRAME_INDEX].Initialize(false, distribution);
+  system_products_[CHROME_FRAME_INDEX].Initialize(true, distribution);
 
-  ActivePackageProperties package_properties;
-  InitializeMultiPackage(false, package_properties,
-      &user_products_[MULTI_PACKAGE_INDEX]);
-  InitializeMultiPackage(true, package_properties,
-      &system_products_[MULTI_PACKAGE_INDEX]);
+  distribution = BrowserDistribution::GetSpecificDistribution(
+      BrowserDistribution::CHROME_BINARIES);
+  user_products_[CHROME_BINARIES_INDEX].Initialize(false, distribution);
+  system_products_[CHROME_BINARIES_INDEX].Initialize(true, distribution);
 }
 
-// static
-void InstallationState::InitializeProduct(bool system_install,
-                                          BrowserDistribution* distribution,
-                                          ProductState* product) {
-  product->Initialize(system_install, distribution->GetVersionKey(),
-      distribution->GetStateKey());
-}
-
-// static
-void InstallationState::InitializeMultiPackage(bool system_install,
-                                               PackageProperties& properties,
-                                               ProductState* product) {
-  product->Initialize(system_install, properties.GetVersionKey(),
-      properties.GetStateKey());
-}
-
-const ProductState* InstallationState::GetMultiPackageState(
-    bool system_install) const {
-  const ProductState& product_state =
-      (system_install ? system_products_ : user_products_)[MULTI_PACKAGE_INDEX];
-  return product_state.version_.get() == NULL ? NULL : &product_state;
-}
-
 const ProductState* InstallationState::GetProductState(
     bool system_install,
     BrowserDistribution::Type type) const {