Product-specific flags are no longer required, used, or validated in --rename-chrome-exe command ...

chrome/installer/setup/setup_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 <windows.h>
 #include <msi.h>
 #include <shellapi.h>
 #include <shlobj.h>
 
+#include <algorithm>
+#include <functional>
 #include <string>
 
 #include "base/at_exit.h"
 #include "base/basictypes.h"
 #include "base/command_line.h"
 #include "base/file_util.h"
 #include "base/file_version_info.h"
 #include "base/memory/scoped_temp_dir.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
@@ skipping 114 matching lines @@
     *archive_type = installer::FULL_ARCHIVE_TYPE;
   }
 
   installer_state.UpdateStage(installer::UNPACKING);
 
   // Unpack the uncompressed archive.
   return LzmaUtil::UnPackArchive(uncompressed_archive.value(),
       output_directory.value(), &unpacked_file);
 }
 
+// In multi-install, adds all products to |installer_state| that are
+// multi-installed and must be updated along with the products already present
+// in |installer_state|.
+void AddExistingMultiInstalls(const InstallationState& original_state,

[grt (UTC plus 2), 2011/05/07 03:18:47]

Note to reviewers: this function was simply moved up in the file.  It was not
changed in any way.

+                              InstallerState* installer_state) {
+  if (installer_state->is_multi_install()) {
+    BrowserDistribution::Type product_checks[] = {

[robertshield, 2011/05/09 14:03:57]

We have occurrences of this particular array in a couple of places, it occurs to
me that this makes it slightly harder to add a new product in the future. Having
the list of known products in a single location feels more maintainable. 
Don't have to do this in this CL, but add a TODO()?

[grt (UTC plus 2), 2011/05/09 14:29:52]

Done.

+      BrowserDistribution::CHROME_BROWSER,
+      BrowserDistribution::CHROME_FRAME
+    };
+
+    for (size_t i = 0; i < arraysize(product_checks); ++i) {
+      BrowserDistribution::Type type = product_checks[i];
+      if (!installer_state->FindProduct(type)) {
+        const ProductState* state =
+            original_state.GetProductState(installer_state->system_install(),
+                                           type);
+        if ((state != NULL) && state->is_multi_install()) {
+          installer_state->AddProductFromState(type, *state);
+          VLOG(1) << "Product already installed and must be included: "
+                  << BrowserDistribution::GetSpecificDistribution(
+                         type)->GetApplicationName();
+        }
+      }
+    }
+  }
+}
+
 // This function is called when --rename-chrome-exe option is specified on
 // setup.exe command line. This function assumes an in-use update has happened
 // for Chrome so there should be a file called new_chrome.exe on the file
 // system and a key called 'opv' in the registry. This function will move
 // new_chrome.exe to chrome.exe and delete 'opv' key in one atomic operation.
 installer::InstallStatus RenameChromeExecutables(
-    const InstallerState& installer_state) {
-  const FilePath &target_path = installer_state.target_path();
+    const InstallationState& original_state,
+    InstallerState* installer_state) {
+  // See what products are already installed in multi mode.  When we do the
+  // rename for multi installs, we must update all installations since they
+  // share the binaries.
+  AddExistingMultiInstalls(original_state, installer_state);
+  const FilePath &target_path = installer_state->target_path();
   FilePath chrome_exe(target_path.Append(installer::kChromeExe));
   FilePath chrome_new_exe(target_path.Append(installer::kChromeNewExe));
   FilePath chrome_old_exe(target_path.Append(installer::kChromeOldExe));
 
   // Create a temporary backup directory on the same volume as chrome.exe so
   // that moving in-use files doesn't lead to trouble.
   installer::SelfCleaningTempDir temp_path;
   if (!temp_path.Initialize(target_path.DirName(),
                             installer::kInstallTempDir)) {
     PLOG(ERROR) << "Failed to create Temp directory "
                 << target_path.DirName()
                        .Append(installer::kInstallTempDir).value();
     return installer::RENAME_FAILED;
   }
   scoped_ptr<WorkItemList> install_list(WorkItem::CreateWorkItemList());
   // Move chrome.exe to old_chrome.exe, then move new_chrome.exe to chrome.exe.
   install_list->AddMoveTreeWorkItem(chrome_exe.value(),
                                     chrome_old_exe.value(),
                                     temp_path.path().value());
   install_list->AddMoveTreeWorkItem(chrome_new_exe.value(),
                                     chrome_exe.value(),
                                     temp_path.path().value());
   install_list->AddDeleteTreeWorkItem(chrome_new_exe, temp_path.path());
   // old_chrome.exe is still in use in most cases, so ignore failures here.
   install_list->AddDeleteTreeWorkItem(chrome_old_exe, temp_path.path())
       ->set_ignore_failure(true);
 
-  HKEY reg_root = installer_state.root_key();
-  const Products& products = installer_state.products();
-  for (size_t i = 0; i < products.size(); ++i) {
-    const Product* product = products[i];
-    BrowserDistribution* browser_dist = product->distribution();
-    std::wstring version_key(browser_dist->GetVersionKey());
+  // Collect the set of distributions we need to update.
+  BrowserDistribution* dists[BrowserDistribution::NUM_TYPES];
+  int num_dists = 0;
+  if (installer_state->is_multi_install())
+    dists[num_dists++] = installer_state->multi_package_binaries_distribution();
+  const Products& products = installer_state->products();
+  num_dists = std::transform(products.begin(), products.end(),
+                             &dists[num_dists],
+                             std::mem_fun(&Product::distribution)) - &dists[0];

[robertshield, 2011/05/09 14:03:57]

The above eight lines are a bit too clever imo. They look like an optimally
concise expression of what you mean but I don't think I could debug this or that
it would make newcomers eager to modify this code.

Could you replace this with a longer but simpler variant or factor it into a
separate documented method explaining how it works?

[grt (UTC plus 2), 2011/05/09 14:29:52]

Done.

+
+  // Add work items to delete the "opv" and "cmd" values from all distributions.
+  HKEY reg_root = installer_state->root_key();
+  std::wstring version_key;
+  for (int i = 0; i < num_dists; ++i) {
+    version_key = dists[i]->GetVersionKey();
     install_list->AddDeleteRegValueWorkItem(reg_root,
                                             version_key,
                                             google_update::kRegOldVersionField);
     install_list->AddDeleteRegValueWorkItem(reg_root,
                                             version_key,
                                             google_update::kRegRenameCmdField);
   }
   installer::InstallStatus ret = installer::RENAME_SUCCESSFUL;
   if (!install_list->Do()) {
     LOG(ERROR) << "Renaming of executables failed. Rolling back any changes.";
@@ skipping 127 matching lines @@
   } else if (DCHECK_IS_ON()) {
     // It isn't possible to stuff two products into a single-install
     // InstallerState.  Abort the process here in debug builds just in case
     // someone finds a way.
     DCHECK_EQ(1U, products.size());
   }
 
   return true;
 }
 
-// In multi-install, adds all products to |installer_state| that are
-// multi-installed and must be updated along with the products already present
-// in |installer_state|.
-void AddExistingMultiInstalls(const InstallationState& original_state,
-                              InstallerState* installer_state) {
-  if (installer_state->is_multi_install()) {
-    BrowserDistribution::Type product_checks[] = {
-      BrowserDistribution::CHROME_BROWSER,
-      BrowserDistribution::CHROME_FRAME
-    };
-
-    for (size_t i = 0; i < arraysize(product_checks); ++i) {
-      BrowserDistribution::Type type = product_checks[i];
-      if (!installer_state->FindProduct(type)) {
-        const ProductState* state =
-            original_state.GetProductState(installer_state->system_install(),
-                                           type);
-        if ((state != NULL) && state->is_multi_install()) {
-          installer_state->AddProductFromState(type, *state);
-          VLOG(1) << "Product already installed and must be included: "
-                  << BrowserDistribution::GetSpecificDistribution(
-                         type)->GetApplicationName();
-        }
-      }
-    }
-  }
-}
-
 // Checks for compatibility between the current state of the system and the
 // desired operation.  Also applies policy that mutates the desired operation;
 // specifically, the |installer_state| object.
 // Also blocks simultaneous user-level and system-level installs.  In the case
 // of trying to install user-level Chrome when system-level exists, the
 // existing system-level Chrome is launched.
 // When the pre-install conditions are not satisfied, the result is written to
 // the registry (via WriteInstallerResult), |status| is set appropriately, and
 // false is returned.
 bool CheckPreInstallConditions(const InstallationState& original_state,
@@ skipping 477 matching lines @@
       }
       *exit_code = ShellUtil::RegisterChromeBrowser(
           chrome_install->distribution(), chrome_exe, suffix, false);
     } else {
       LOG(DFATAL) << "Can't register browser - Chrome distribution not found";
       *exit_code = installer::UNKNOWN_STATUS;
     }
   } else if (cmd_line.HasSwitch(installer::switches::kRenameChromeExe)) {
     // If --rename-chrome-exe is specified, we want to rename the executables
     // and exit.
-    *exit_code = RenameChromeExecutables(*installer_state);
+    *exit_code = RenameChromeExecutables(original_state, installer_state);
   } else if (cmd_line.HasSwitch(
       installer::switches::kRemoveChromeRegistration)) {
     // This is almost reverse of --register-chrome-browser option above.
     // Here we delete Chrome browser registration. This option should only
     // be used when setup.exe is launched with admin rights. We do not
     // make any user specific changes in this option.
     std::wstring suffix;
     if (cmd_line.HasSwitch(
         installer::switches::kRegisterChromeBrowserSuffix)) {
       suffix = cmd_line.GetSwitchValueNative(
@@ skipping 340 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;
 }