C++ reability review for gab.

chrome/installer/setup/install_worker.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.
 //
 // This file contains the definitions of the installer functions that build
 // the WorkItemList used to install the application.
 
+//Pickup file for readability review.
+
 #include "chrome/installer/setup/install_worker.h"
 
 #include <oaidl.h>
 #include <shlobj.h>
 #include <time.h>
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/command_line.h"
@@ skipping 63 matching lines @@
                               arraysize(cf_clsid));
     key_path->append(&cf_clsid[0], len - 1);
   } else {
     key_path->append(kIELowRightsPolicyOldGuid,
                      arraysize(kIELowRightsPolicyOldGuid)- 1);
   }
 }
 
 // Local helper to call AddRegisterComDllWorkItems for all DLLs in a set of
 // products managed by a given package.
+// |old_version| can be NULL to indicate no Chrome is currently installed.
 void AddRegisterComDllWorkItemsForPackage(const InstallerState& installer_state,
                                           const Version* old_version,
                                           const Version& new_version,
                                           WorkItemList* work_item_list) {
   // First collect the list of DLLs to be registered from each product.
   std::vector<FilePath> com_dll_list;
   installer_state.AddComDllList(&com_dll_list);
 
   // Then, if we got some, attempt to unregister the DLLs from the old
   // version directory and then re-register them in the new one.
@@ skipping 62 matching lines @@
     // setup.exe cannot be run again without regenerating the archive, so copy
     // it instead in this case to save developer time.
     install_list->AddCopyTreeWorkItem(archive_path.value(), archive_dst.value(),
                                       temp_path.value(), WorkItem::ALWAYS);
 #endif  // COMPONENT_BUILD
   }
 }
 
 void AddInstallAppCommandWorkItems(const InstallerState& installer_state,
                                    const InstallationState& machine_state,
-                                   const FilePath* setup_path,
-                                   const Version* new_version,
+                                   const FilePath& setup_path,
+                                   const Version& new_version,
                                    const Product& product,
                                    WorkItemList* work_item_list) {
   DCHECK(product.is_chrome_app_host());
 
   string16 cmd_key(product.distribution()->GetVersionKey());
   cmd_key.append(1, L'\\').append(google_update::kRegCommandsKey)
       .append(1, L'\\').append(kCmdInstallApp);
 
-  if (installer_state.operation() != InstallerState::UNINSTALL) {
-    FilePath target_path(installer_state.target_path());
-    CommandLine cmd_line(target_path.Append(installer::kChromeAppHostExe));
+  if (installer_state.operation() == InstallerState::UNINSTALL) {
+    work_item_list->AddDeleteRegKeyWorkItem(
+        installer_state.root_key(), cmd_key)->set_log_message(
+            "removing install-application command");
+  } else {
+    CommandLine cmd_line(
+        installer_state.target_path().Append(installer::kChromeAppHostExe));
     cmd_line.AppendSwitchASCII(::switches::kAppsInstallFromManifestURL, "%1");
 
     AppCommand cmd(cmd_line.GetCommandLineString());
     cmd.set_sends_pings(true);
     cmd.set_is_web_accessible(true);
     cmd.AddWorkItems(installer_state.root_key(), cmd_key, work_item_list);
-  } else {
-    work_item_list->AddDeleteRegKeyWorkItem(installer_state.root_key(),
-                                            cmd_key)->set_log_message(
-        "removing install-application command");
   }
 }
 
 void AddProductSpecificWorkItems(const InstallationState& original_state,
                                  const InstallerState& installer_state,
                                  const FilePath& setup_path,
                                  const Version& new_version,
                                  WorkItemList* list) {
   const Products& products = installer_state.products();
-  for (size_t i = 0; i < products.size(); ++i) {
-    const Product& p = *products[i];
+  for (Products::const_iterator it = products.begin(); it < products.end();
+       ++it) {
+    const Product& p = **it;
     if (p.is_chrome_frame()) {
       AddChromeFrameWorkItems(original_state, installer_state, setup_path,
                               new_version, p, list);
     }
     if (p.is_chrome_app_host()) {
       AddInstallAppCommandWorkItems(installer_state, original_state,
-                                    &setup_path, &new_version, p, list);
+                                    setup_path, new_version, p, list);
     }
     if (p.is_chrome()) {
-      AddOsUpgradeWorkItems(installer_state, &setup_path, &new_version, p,
+      AddOsUpgradeWorkItems(installer_state, setup_path, new_version, p,
                             list);
     }
   }
 }
 
 // This is called when an MSI installation is run. It may be that a user is
 // attempting to install the MSI on top of a non-MSI managed installation.
 // If so, try and remove any existing uninstallation shortcuts, as we want the
 // uninstall to be managed entirely by the MSI machinery (accessible via the
 // Add/Remove programs dialog).
@@ skipping 31 matching lines @@
     VLOG(1) << "Deleting old uninstall shortcut (if present): "
             << uninstall_link.value();
     WorkItem* delete_link = work_item_list->AddDeleteTreeWorkItem(
         uninstall_link, temp_path);
     delete_link->set_ignore_failure(true);
     delete_link->set_log_message(
         "Failed to delete old uninstall shortcut.");
   }
 }
 
+// Adds Chrome specific install work items to |install_list|.
+// |current_version| can be NULL to indicate no Chrome is currently installed.
 void AddChromeWorkItems(const InstallationState& original_state,
                         const InstallerState& installer_state,
                         const FilePath& setup_path,
                         const FilePath& archive_path,
                         const FilePath& src_path,
                         const FilePath& temp_path,
+                        const Version* current_version,
                         const Version& new_version,
-                        scoped_ptr<Version>* current_version,
                         WorkItemList* install_list) {
   const FilePath& target_path = installer_state.target_path();
 
-  if (current_version != NULL && current_version->get() != NULL) {
+  if (current_version) {
     // Delete the archive from an existing install to save some disk space.  We
     // make this an unconditional work item since there's no need to roll this
     // back; if installation fails we'll be moved to the "-full" channel anyway.
     FilePath old_installer_dir(
-        installer_state.GetInstallerDirectory(**current_version));
+        installer_state.GetInstallerDirectory(*current_version));
     FilePath old_archive(old_installer_dir.Append(installer::kChromeArchive));
     // Don't delete the archive that we are actually installing from.
     if (archive_path != old_archive) {
-      install_list->AddDeleteTreeWorkItem(old_archive, temp_path)
-          ->set_ignore_failure(true);
+      install_list->AddDeleteTreeWorkItem(old_archive, temp_path)->
+          set_ignore_failure(true);
     }
   }
 
   // Delete any new_chrome.exe if present (we will end up creating a new one
   // if required) and then copy chrome.exe
   FilePath new_chrome_exe(target_path.Append(installer::kChromeNewExe));
 
   install_list->AddDeleteTreeWorkItem(new_chrome_exe, temp_path);
 
   if (installer_state.IsChromeFrameRunning(original_state)) {
@@ skipping 63 matching lines @@
       temp_path.value(),
       WorkItem::ALWAYS_MOVE);
 #endif  // defined(COMPONENT_BUILD)
 
   // In the past, we copied rather than moved for system level installs so that
   // the permissions of %ProgramFiles% would be picked up.  Now that |temp_path|
   // is in %ProgramFiles% for system level installs (and in %LOCALAPPDATA%
   // otherwise), there is no need to do this.
   // Note that we pass true for check_duplicates to avoid failing on in-use
   // repair runs if the current_version is the same as the new_version.
-  bool check_for_duplicates =
-      (current_version != NULL && current_version->get() != NULL &&
-       current_version->get()->Equals(new_version));
+  bool check_for_duplicates = (current_version &&
+                               current_version->Equals(new_version));
   install_list->AddMoveTreeWorkItem(
       src_path.AppendASCII(new_version.GetString()).value(),
       target_path.AppendASCII(new_version.GetString()).value(),
       temp_path.value(),
       check_for_duplicates ? WorkItem::CHECK_DUPLICATES :
                              WorkItem::ALWAYS_MOVE);
 
   // Delete any old_chrome.exe if present (ignore failure if it's in use).
   install_list->AddDeleteTreeWorkItem(
-      target_path.Append(installer::kChromeOldExe), temp_path)
-      ->set_ignore_failure(true);
+      target_path.Append(installer::kChromeOldExe), temp_path)->
+          set_ignore_failure(true);
 
   // Copy installer in install directory and
   // add shortcut in Control Panel->Add/Remove Programs.
   AddInstallerCopyTasks(installer_state, setup_path, archive_path, temp_path,
                         new_version, install_list);
 }
 
 // Probes COM machinery to get an instance of delegate_execute.exe's
 // CommandExecuteImpl class.  This is required so that COM purges its cache of
 // the path to the binary, which changes on updates.  This callback
@@ skipping 19 matching lines @@
       LOG(ERROR) << "Unexpected result creating CommandExecuteImpl; hr=0x"
                  << std::hex << hr;
     }
   }
 
   return true;
 }
 
 void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,
                                     const InstallationState& machine_state,
-                                    const FilePath* setup_path,
-                                    const Version* new_version,
-                                    WorkItemList* work_item_list,
+                                    const FilePath& setup_path,
+                                    const Version& new_version,
                                     bool have_child_product,
                                     const CommandLine& child_product_switches,
-                                    const string16& command_id) {
-  DCHECK(setup_path ||
-         installer_state.operation() == InstallerState::UNINSTALL);
-  DCHECK(new_version ||
-         installer_state.operation() == InstallerState::UNINSTALL);
+                                    const string16& command_id,
+                                    WorkItemList* work_item_list) {
   DCHECK(work_item_list);
 
   const bool system_install = installer_state.system_install();
   bool have_chrome_binaries = false;
 
   // STEP 1: Figure out the state of the machine before the operation.
   const ProductState* product_state = NULL;
 
   // Are the Chrome Binaries already on the machine?
-  product_state =
-      machine_state.GetProductState(system_install,
-                                    BrowserDistribution::CHROME_BINARIES);
+  product_state = machine_state.GetProductState(
+      system_install, BrowserDistribution::CHROME_BINARIES);
   if (product_state != NULL && product_state->is_multi_install())
     have_chrome_binaries = true;
 
   // STEP 2: Now take into account the current operation.
   const Product* product = NULL;
 
   if (installer_state.operation() == InstallerState::UNINSTALL) {
     // Forget about multi-install Chrome if it is being uninstalled.
-    product =
-        installer_state.FindProduct(BrowserDistribution::CHROME_BINARIES);
+    product = installer_state.FindProduct(BrowserDistribution::CHROME_BINARIES);
     if (product != NULL && installer_state.is_multi_install())
       have_chrome_binaries = false;
   } else {
     // Check if we're installing Chrome Binaries
-    product =
-        installer_state.FindProduct(BrowserDistribution::CHROME_BINARIES);
+    product = installer_state.FindProduct(BrowserDistribution::CHROME_BINARIES);
     if (product != NULL && installer_state.is_multi_install())
       have_chrome_binaries = true;
   }
 
   // STEP 3: Decide what to do based on the final state of things.
   enum QuickEnableOperation {
     DO_NOTHING,
     ADD_COMMAND,
     REMOVE_COMMAND
   } operation = DO_NOTHING;
   FilePath binaries_setup_path;
 
   if (have_child_product) {
     // Child product is being uninstalled. Unconditionally remove the Quick
     // Enable command from the binaries. We do this even if multi-install Chrome
     // isn't installed since we don't want them left behind in any case.
     operation = REMOVE_COMMAND;
   } else if (have_chrome_binaries) {
     // Child product isn't (to be) installed while multi-install Chrome is (to
     // be) installed.  Add the Quick Enable command to the binaries.
     operation = ADD_COMMAND;
     // The path to setup.exe contains the version of the Chrome binaries, so it
     // takes a little work to get it right.
     if (installer_state.operation() == InstallerState::UNINSTALL) {
       // One or more products are being uninstalled, but not the binaries. Use
       // the path to the currently installed Chrome setup.exe.
-      product_state =
-          machine_state.GetProductState(system_install,
-                                        BrowserDistribution::CHROME_BINARIES);
+      product_state = machine_state.GetProductState(
+          system_install, BrowserDistribution::CHROME_BINARIES);
       DCHECK(product_state);
       binaries_setup_path = product_state->uninstall_command().GetProgram();
     } else {
       // Chrome Binaries are being installed, updated, or otherwise operated on.
       // Use the path to the given |setup_path| in the normal location of
       // multi-install Chrome Binaries of the given |version|.
       DCHECK(installer_state.is_multi_install());
       binaries_setup_path =
-          installer_state.GetInstallerDirectory(*new_version).Append(
-              setup_path->BaseName());
+          installer_state.GetInstallerDirectory(new_version).Append(
+              setup_path.BaseName());
     }
   }
 
   // STEP 4: Take action.
   if (operation != DO_NOTHING) {
     // Get the path to the quick-enable-cf command for the binaries.
     BrowserDistribution* binaries =
         BrowserDistribution::GetSpecificDistribution(
             BrowserDistribution::CHROME_BINARIES);
     string16 cmd_key(binaries->GetVersionKey());
     cmd_key.append(1, L'\\').append(google_update::kRegCommandsKey)
         .append(1, L'\\').append(command_id);
 
     if (operation == ADD_COMMAND) {
       DCHECK(!binaries_setup_path.empty());
       CommandLine cmd_line(binaries_setup_path);
       cmd_line.AppendArguments(child_product_switches,
                                false);  // include_program
       if (installer_state.verbose_logging())
         cmd_line.AppendSwitch(switches::kVerboseLogging);
       AppCommand cmd(cmd_line.GetCommandLineString());
       cmd.set_sends_pings(true);
       cmd.set_is_web_accessible(true);
       cmd.AddWorkItems(installer_state.root_key(), cmd_key, work_item_list);
     } else {
       DCHECK(operation == REMOVE_COMMAND);
-      work_item_list->AddDeleteRegKeyWorkItem(installer_state.root_key(),
-                                              cmd_key)->set_log_message(
-          "removing " + WideToASCII(command_id) + " command");
+      work_item_list->AddDeleteRegKeyWorkItem(
+          installer_state.root_key(), cmd_key)->set_log_message(
+              "removing " + WideToASCII(command_id) + " command");
     }
   }
 }
 
 }  // namespace
 
 // This method adds work items to create (or update) Chrome uninstall entry in
 // either the Control Panel->Add/Remove Programs list or in the Omaha client
 // state key if running under an MSI installer.
 void AddUninstallShortcutWorkItems(const InstallerState& installer_state,
                                    const FilePath& setup_path,
                                    const Version& new_version,
-                                   WorkItemList* install_list,
-                                   const Product& product) {
+                                   const Product& product,
+                                   WorkItemList* install_list) {
   HKEY reg_root = installer_state.root_key();
   BrowserDistribution* browser_dist = product.distribution();
   DCHECK(browser_dist);
 
   // When we are installed via an MSI, we need to store our uninstall strings
   // in the Google Update client state key. We do this even for non-MSI
   // managed installs to avoid breaking the edge case whereby an MSI-managed
   // install is updated by a non-msi installer (which would confuse the MSI
   // machinery if these strings were not also updated). The UninstallString
   // value placed in the client state key is also used by the mini_installer to
@@ skipping 152 matching lines @@
       // Nothing is being installed?  Entirely unexpected, so do no harm.
       LOG(ERROR) << "No products found in AddOemInstallWorkItems";
       return;
     }
     const ProductState* source_product =
         original_state.GetNonVersionedProductState(system_install, source_type);
 
     string16 oem_install;
     if (source_product->GetOemInstall(&oem_install)) {
       VLOG(1) << "Mirroring oeminstall=\"" << oem_install << "\" from "
-              << BrowserDistribution::GetSpecificDistribution(source_type)
-                     ->GetAppShortCutName();
+              << BrowserDistribution::GetSpecificDistribution(source_type)->
+                    GetAppShortCutName();
       install_list->AddCreateRegKeyWorkItem(root_key, multi_key);
       // Always overwrite an old value.
       install_list->AddSetRegValueWorkItem(root_key, multi_key,
                                            google_update::kRegOemInstallField,
                                            oem_install, true);
     } else {
       // Clear any old value.
       install_list->AddDeleteRegValueWorkItem(
           root_key, multi_key, google_update::kRegOemInstallField);
     }
   }
 }
 
 // Mirror eulaaccepted the first time anything is installed multi.  There is no

[dominich, 2012/09/10 15:40:42]

nit: single space after period.

1 other places in this file.

[gab, 2012/09/10 19:14:05]

This is grt@'s comment/grammar style, he has been doing that for longer than
I've been at Google (and maybe even than I've been coding!). I'll be happy to
change this if you can convince him this is the right thing to do.

[grt (UTC plus 2), 2012/09/10 19:59:30]

Get out of my yard.

[dominich, 2012/09/10 20:33:46]

Challenge accepted!

Use of correct punctuation, spelling, and grammar is explicitly mentioned in the
style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Punctu...

Consulting the two leading manuals of style and grammar for published works:
http://www.mla.org/style_faq3
http://www.chicagomanualofstyle.org/CMS_FAQ/OneSpaceorTwo/OneSpaceorTwo03.html

Given also the limitation of 80 columns, preserving horizontal whitespace should
be seen as important as preserving vertical whitespace.

However, it's just a nit.

 // need to update the value on future install/update runs since
 // GoogleUpdateSettings::SetEULAConsent will modify the value for both the
 // relevant product and for the binaries.
 void AddEulaAcceptedWorkItems(const InstallationState& original_state,
                               const InstallerState& installer_state,
                               WorkItemList* install_list) {
   DCHECK(installer_state.is_multi_install());
   const bool system_install = installer_state.system_install();
   if (!original_state.GetProductState(system_install,
                                       BrowserDistribution::CHROME_BINARIES)) {
     const HKEY root_key = installer_state.root_key();
     string16 multi_key(
         installer_state.multi_package_binaries_distribution()->GetStateKey());
 
     // Copy the value from the product with the greatest value.
     bool have_eula_accepted = false;
     BrowserDistribution::Type product_type;
     DWORD eula_accepted;
     const Products& products = installer_state.products();
-    for (size_t i = 0, count = products.size(); i != count; ++i) {
-      if (products[i]->is_chrome_binaries())
+    for (Products::const_iterator it = products.begin(); it < products.end();
+         ++it) {
+      const Product& product = **it;
+      if (product.is_chrome_binaries())
         continue;
       DWORD dword_value = 0;
       BrowserDistribution::Type this_type =

[dominich, 2012/09/10 15:40:42]

nit - this fits on one line.

[gab, 2012/09/10 19:14:05]

Done.

-          products[i]->distribution()->GetType();
+          product.distribution()->GetType();
       const ProductState* product_state =
           original_state.GetNonVersionedProductState(
               system_install, this_type);
       if (product_state->GetEulaAccepted(&dword_value) &&
           (!have_eula_accepted || dword_value > eula_accepted)) {
         have_eula_accepted = true;
         eula_accepted = dword_value;
         product_type = this_type;
       }
     }
 
     if (have_eula_accepted) {
       VLOG(1) << "Mirroring eulaaccepted=" << eula_accepted << " from "
-              << BrowserDistribution::GetSpecificDistribution(product_type)
-                     ->GetAppShortCutName();
+              << BrowserDistribution::GetSpecificDistribution(product_type)->
+                     GetAppShortCutName();
       install_list->AddCreateRegKeyWorkItem(root_key, multi_key);
       install_list->AddSetRegValueWorkItem(
           root_key, multi_key, google_update::kRegEULAAceptedField,
           eula_accepted, true);
     } else {
       // Clear any old value.
       install_list->AddDeleteRegValueWorkItem(
           root_key, multi_key, google_update::kRegEULAAceptedField);
     }
   }
@@ skipping 39 matching lines @@
       // if one is already present (although this shouldn't happen).
       install_list->AddSetRegValueWorkItem(root_key,
                                            multi_key,
                                            google_update::kRegBrandField,
                                            brand,
                                            false);
     }
   }
 
   AddOemInstallWorkItems(original_state, installer_state, install_list);
 

[dominich, 2012/09/10 15:40:42]

nit: remove blank lines here and 834

[gab, 2012/09/10 19:14:05]

Done.

   AddEulaAcceptedWorkItems(original_state, installer_state, install_list);
 
   AddUsageStatsWorkItems(original_state, installer_state, install_list);
 
   // TODO(grt): check for other keys/values we should put in the package's
   // ClientState and/or Clients key.
 }
 
 void AddUsageStatsWorkItems(const InstallationState& original_state,
                             const InstallerState& installer_state,
@@ skipping 39 matching lines @@
         install_list->AddDeleteRegValueWorkItem(
             root_key, dist->GetStateMediumKey(),
             google_update::kRegUsageStatsField);
         // Previous versions of Chrome also wrote a value in HKCU even for
         // system-level installs, so clean that up.
         install_list->AddDeleteRegValueWorkItem(
             HKEY_CURRENT_USER, dist->GetStateKey(),
             google_update::kRegUsageStatsField);
       }
       install_list->AddDeleteRegValueWorkItem(root_key, dist->GetStateKey(),
           google_update::kRegUsageStatsField);

[dominich, 2012/09/10 15:40:42]

this should be aligned to root_key, or the first two params should be dropped a
line.

[gab, 2012/09/10 19:14:05]

Done.

     }
   }
 }
 
-// After a successful copying of all the files, this function is called to
-// do a few post install tasks:
-// - Handle the case of in-use-update by updating "opv" (old version) key or
-//   deleting it if not required.
-// - Register any new dlls and unregister old dlls.
-// - If this is an MSI install, ensures that the MSI marker is set, and sets
-//   it if not.
-// If these operations are successful, the function returns true, otherwise
-// false.
 bool AppendPostInstallTasks(const InstallerState& installer_state,
                             const FilePath& setup_path,
                             const Version* current_version,
                             const Version& new_version,
                             const FilePath& temp_path,
                             WorkItemList* post_install_task_list) {
   DCHECK(post_install_task_list);
 
   HKEY root = installer_state.root_key();
   const Products& products = installer_state.products();
@@ skipping 23 matching lines @@
       rename.AppendSwitch(switches::kSystemLevel);
 
     if (installer_state.verbose_logging())
       rename.AppendSwitch(switches::kVerboseLogging);
 
     string16 version_key;
     for (size_t i = 0; i < products.size(); ++i) {
       BrowserDistribution* dist = products[i]->distribution();
       version_key = dist->GetVersionKey();
 
-      if (current_version != NULL) {
+      if (current_version) {
         in_use_update_work_items->AddSetRegValueWorkItem(root, version_key,
             google_update::kRegOldVersionField,
             ASCIIToWide(current_version->GetString()), true);
       }
       if (critical_version.IsValid()) {
         in_use_update_work_items->AddSetRegValueWorkItem(root, version_key,
             google_update::kRegCriticalVersionField,
             ASCIIToWide(critical_version.GetString()), true);
       } else {
         in_use_update_work_items->AddDeleteRegValueWorkItem(root, version_key,
@@ skipping 74 matching lines @@
 
   return true;
 }
 
 void AddInstallWorkItems(const InstallationState& original_state,
                          const InstallerState& installer_state,
                          const FilePath& setup_path,
                          const FilePath& archive_path,
                          const FilePath& src_path,
                          const FilePath& temp_path,
+                         const Version* current_version,
                          const Version& new_version,
-                         scoped_ptr<Version>* current_version,
                          WorkItemList* install_list) {
   DCHECK(install_list);
 
   const FilePath& target_path = installer_state.target_path();
 
   // A temp directory that work items need and the actual install directory.
   install_list->AddCreateDirWorkItem(temp_path);
   install_list->AddCreateDirWorkItem(target_path);
 
   if (installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER) ||
       installer_state.FindProduct(BrowserDistribution::CHROME_FRAME) ||
       installer_state.FindProduct(BrowserDistribution::CHROME_BINARIES)) {
     AddChromeWorkItems(original_state,
                        installer_state,
                        setup_path,
                        archive_path,
                        src_path,
                        temp_path,
+                       current_version,
                        new_version,
-                       current_version,
                        install_list);
   }
 
   if (installer_state.FindProduct(BrowserDistribution::CHROME_APP_HOST)) {
     install_list->AddCopyTreeWorkItem(
         src_path.Append(installer::kChromeAppHostExe).value(),
         target_path.Append(installer::kChromeAppHostExe).value(),
         temp_path.value(),
         WorkItem::ALWAYS,
         L"");
   }
 
   const HKEY root = installer_state.root_key();
   // Only set "lang" for user-level installs since for system-level, the install
   // language may not be related to a given user's runtime language.
   const bool add_language_identifier = !installer_state.system_install();
 
   const Products& products = installer_state.products();
-  for (size_t i = 0; i < products.size(); ++i) {
-    const Product* product = products[i];
+  for (Products::const_iterator it = products.begin(); it < products.end();
+       ++it) {
+    const Product& product = **it;
 
     AddUninstallShortcutWorkItems(installer_state, setup_path, new_version,
-                                  install_list, *product);
+                                  product, install_list);
 
-    AddVersionKeyWorkItems(root, product->distribution(), new_version,
+    AddVersionKeyWorkItems(root, product.distribution(), new_version,
                            add_language_identifier, install_list);
 
     AddDelegateExecuteWorkItems(installer_state, src_path, new_version,
-                                *product, install_list);
+                                product, install_list);
 
 // TODO(gab): This is only disabled for M22 as the shortcut CL using Active
 // Setup will not make it in M22.
 #if 0
     AddActiveSetupWorkItems(installer_state, new_version, *product,
                             install_list);
 #endif
   }
 
   // Add any remaining work items that involve special settings for
   // each product.
   AddProductSpecificWorkItems(original_state, installer_state, setup_path,
                               new_version, install_list);
 
   // Copy over brand, usagestats, and other values.
   AddGoogleUpdateWorkItems(original_state, installer_state, install_list);
 
   AddQuickEnableApplicationHostWorkItems(installer_state, original_state,
-                                         &setup_path, &new_version,
+                                         setup_path, new_version,
                                          install_list);
 
   AddQuickEnableChromeFrameWorkItems(installer_state, original_state,
-                                     &setup_path, &new_version, install_list);
+                                     setup_path, new_version, install_list);
 
   // Append the tasks that run after the installation.
   AppendPostInstallTasks(installer_state,
                          setup_path,
-                         current_version->get(),
+                         current_version,
                          new_version,
                          temp_path,
                          install_list);
 }
 
 void AddRegisterComDllWorkItems(const FilePath& dll_folder,
                                 const std::vector<FilePath>& dll_list,
                                 bool system_level,
                                 bool do_register,
                                 bool ignore_failures,
@@ skipping 48 matching lines @@
   BrowserDistribution* dist =
       installer_state.multi_package_binaries_distribution();
   if (ready_mode) {
     // If GCF is being installed in ready mode, we write an entry to the
     // multi-install state key.  If the value already exists, we will not
     // overwrite it since the user might have opted out.
     list->AddCreateRegKeyWorkItem(root, dist->GetStateKey());
     list->AddSetRegValueWorkItem(root, dist->GetStateKey(),
         kChromeFrameReadyModeField,
         static_cast<int64>(is_install ? 1 : 0),  // The value we want to set.
-        is_install ? false : true);  // Overwrite existing value.
+        !is_install);  // Overwrite existing value.
     if (is_install) {
       FilePath installer_path(installer_state
           .GetInstallerDirectory(new_version).Append(setup_path.BaseName()));
 
       CommandLine basic_cl(installer_path);
       basic_cl.AppendSwitch(switches::kChromeFrame);
       basic_cl.AppendSwitch(switches::kMultiInstall);
 
       if (installer_state.system_install())
         basic_cl.AppendSwitch(switches::kSystemLevel);
@@ skipping 74 matching lines @@
                                     google_update::kRegCFOptOutCmdField);
     list->AddDeleteRegValueWorkItem(root, version_key,
                                     google_update::kRegCFOptInCmdField);
   }
 
   if (update_chrome_uninstall_command) {
     // Chrome is not a part of this installation run, so we have to explicitly
     // check if Chrome is installed, and if so, update its uninstallation
     // command lines.
     const ProductState* chrome_state = original_state.GetProductState(
-        installer_state.system_install(),
-        BrowserDistribution::CHROME_BROWSER);
+        installer_state.system_install(), BrowserDistribution::CHROME_BROWSER);
     if (chrome_state != NULL) {
       DCHECK(chrome_state->is_multi_install());
       Product chrome(BrowserDistribution::GetSpecificDistribution(
                          BrowserDistribution::CHROME_BROWSER));
       chrome.InitializeFromUninstallCommand(chrome_state->uninstall_command());
       AddUninstallShortcutWorkItems(installer_state, setup_path,
-                                    chrome_state->version(), list, chrome);
+                                    chrome_state->version(), chrome, list);
     } else {
       NOTREACHED() << "What happened to Chrome?";
     }
   }
 }
 
 void AddDelegateExecuteWorkItems(const InstallerState& installer_state,
                                  const FilePath& src_path,
                                  const Version& new_version,
                                  const Product& product,
@@ skipping 184 matching lines @@
     }
 
     FreeLibrary(ieframe);
   } else {
     VLOG(1) << "Cannot load " << kIEFrameDll;
   }
 }
 
 void AddQuickEnableChromeFrameWorkItems(const InstallerState& installer_state,
                                         const InstallationState& machine_state,
-                                        const FilePath* setup_path,
-                                        const Version* new_version,
+                                        const FilePath& setup_path,
+                                        const Version& new_version,
                                         WorkItemList* work_item_list) {
-  DCHECK(setup_path ||
-         installer_state.operation() == InstallerState::UNINSTALL);
-  DCHECK(new_version ||
-         installer_state.operation() == InstallerState::UNINSTALL);
   DCHECK(work_item_list);
 
   const bool system_install = installer_state.system_install();
   bool have_chrome_frame = false;
 
   // STEP 1: Figure out the state of the machine before the operation.
   const ProductState* product_state = NULL;
 
   // Is Chrome Frame !ready-mode already on the machine?
-  product_state =
-      machine_state.GetProductState(system_install,
-                                    BrowserDistribution::CHROME_FRAME);
+  product_state = machine_state.GetProductState(
+      system_install, BrowserDistribution::CHROME_FRAME);
   if (product_state != NULL &&
       !product_state->uninstall_command().HasSwitch(
           switches::kChromeFrameReadyMode))
     have_chrome_frame = true;
 
   // STEP 2: Now take into account the current operation.
   const Product* product = NULL;
 
   if (installer_state.operation() == InstallerState::UNINSTALL) {
     // Forget about Chrome Frame if it is being uninstalled.  Note that we don't
@@ skipping 14 matching lines @@
   CommandLine cmd_line(CommandLine::NO_PROGRAM);
   cmd_line.AppendSwitch(switches::kMultiInstall);
   if (installer_state.system_install())
     cmd_line.AppendSwitch(switches::kSystemLevel);
   cmd_line.AppendSwitch(switches::kChromeFrameQuickEnable);
 
   AddGenericQuickEnableWorkItems(installer_state,
                                  machine_state,
                                  setup_path,
                                  new_version,
-                                 work_item_list,
                                  have_chrome_frame,
                                  cmd_line,
-                                 kCmdQuickEnableCf);
+                                 kCmdQuickEnableCf,
+                                 work_item_list);
 }
 
 void AddQuickEnableApplicationHostWorkItems(
     const InstallerState& installer_state,
     const InstallationState& machine_state,
-    const FilePath* setup_path,
-    const Version* new_version,
+    const FilePath& setup_path,
+    const Version& new_version,
     WorkItemList* work_item_list) {
-  DCHECK(setup_path ||
-         installer_state.operation() == InstallerState::UNINSTALL);
-  DCHECK(new_version ||
-         installer_state.operation() == InstallerState::UNINSTALL);
   DCHECK(work_item_list);
 
   CommandLine cmd_line(CommandLine::NO_PROGRAM);
   cmd_line.AppendSwitch(switches::kMultiInstall);
   cmd_line.AppendSwitch(switches::kChromeAppHost);
 
   // For system-level binaries there is no way to keep the command state in sync
   // with the installation/uninstallation of the Application Host (which is
   // always at user-level).
   // So we pass false for 'have_child_product' to cause this command to always
   // be installed if the Chrome Binaries are installed.
   AddGenericQuickEnableWorkItems(installer_state,
                                  machine_state,
                                  setup_path,
                                  new_version,
-                                 work_item_list,
                                  false,  // have_child_product
                                  cmd_line,
-                                 kCmdQuickEnableApplicationHost);
+                                 kCmdQuickEnableApplicationHost,
+                                 work_item_list);
 }
 
 void AddOsUpgradeWorkItems(const InstallerState& installer_state,
-                           const FilePath* setup_path,
-                           const Version* new_version,
+                           const FilePath& setup_path,
+                           const Version& new_version,
                            const Product& product,
                            WorkItemList* install_list) {
   const HKEY root_key = installer_state.root_key();
   string16 cmd_key(product.distribution()->GetVersionKey());
   cmd_key.append(1, FilePath::kSeparators[0])
       .append(google_update::kRegCommandsKey)
       .append(1, FilePath::kSeparators[0])
       .append(kCmdOnOsUpgrade);
 
-  // This will make Google Update call setup.exe with --on-os-upgrade switch.
-  // For Chrome, this leads to HandleOsUpgradeForBrowser() being called.
-  if (installer_state.operation() != InstallerState::UNINSTALL) {
+  if (installer_state.operation() == InstallerState::UNINSTALL) {
+    install_list->AddDeleteRegKeyWorkItem(root_key, cmd_key)->
+        set_log_message("Removing OS upgrade command");
+  } else {
+    // Register with Google Update to have setup.exe --on-os-upgrade called on
+    // OS upgrade.
     CommandLine cmd_line(installer_state
-        .GetInstallerDirectory(*new_version)
-        .Append(setup_path->BaseName()));
+        .GetInstallerDirectory(new_version)
+        .Append(setup_path.BaseName()));
     // Add the main option to indicate OS upgrade flow.
     cmd_line.AppendSwitch(installer::switches::kOnOsUpgrade);
     // Add product-specific options.
     product.AppendProductFlags(&cmd_line);
     if (installer_state.system_install())
       cmd_line.AppendSwitch(installer::switches::kSystemLevel);
     // Log everything for now.
     cmd_line.AppendSwitch(installer::switches::kVerboseLogging);
 
     AppCommand cmd(cmd_line.GetCommandLineString());
     cmd.set_is_auto_run_on_os_upgrade(true);
     cmd.AddWorkItems(installer_state.root_key(), cmd_key, install_list);
-  } else {
-    install_list->AddDeleteRegKeyWorkItem(root_key, cmd_key)
-        ->set_log_message("Removing OS upgrade command");
   }
 }
 
 }  // namespace installer