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

chrome/installer/setup/setup_main.cc

Index: chrome/installer/setup/setup_main.cc
===================================================================
--- chrome/installer/setup/setup_main.cc	(revision 84528)
+++ chrome/installer/setup/setup_main.cc	(working copy)
@@ -7,6 +7,8 @@
 #include <shellapi.h>
 #include <shlobj.h>
 
+#include <algorithm>
+#include <functional>
 #include <string>
 
 #include "base/at_exit.h"
@@ -141,14 +143,47 @@
       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));
@@ -176,12 +211,21 @@
   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);
@@ -329,34 +373,6 @@
   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.
@@ -854,7 +870,7 @@
   } 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.