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 267 matching lines @@
                                    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) {

[dominich, 2012/09/06 17:26:12]

Consider swapping these conditionals so the positive check is done first. Ie

if (installer_state.operation() == InstallerState::UNINSTALL) {
...

This isn't in the style guide, but is generally more readable.

1 other place in this file.

[gab, 2012/09/07 20:56:44]

Done.

     FilePath target_path(installer_state.target_path());
     CommandLine cmd_line(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");

[dominich, 2012/09/06 17:26:12]

more readable:

work_item_list->AddDeleteRegKeyWorkItem(
    installer_state.root_key(), cmd_key)->set_log_message(
        "removing install-application command");

[gab, 2012/09/07 20:56:44]

Done.

   }
 }
 
 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) {

[dominich, 2012/09/06 17:26:12]

consider using an iterator for the loop here as it saves accessing the size
every iteration and simplifies the *products[i] on the next line.

1 other place in this file.

[gab, 2012/09/07 20:56:44]

Done.

     const Product& p = *products[i];
     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);
     }
     if (p.is_chrome()) {
@@ skipping 417 matching lines @@
 
   if (current_version != NULL && current_version->get() != NULL) {
     // 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));
     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)

[dominich, 2012/09/06 17:26:12]

nit: can you move the -> up to this line? it makes it clearer that the line
continues.

2+ other places in this file.

[gab, 2012/09/07 20:56:44]

Done.

           ->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);
 
@@ skipping 77 matching lines @@
   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);

[dominich, 2012/09/06 17:26:12]

nit: 4 space indent.

[gab, 2012/09/07 20:56:44]

Done.

 
   // 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);
 
 }
 
 void AddInstallWorkItems(const InstallationState& original_state,
                          const InstallerState& installer_state,
@@ skipping 141 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.

[dominich, 2012/09/06 17:26:12]

nit: !is_install

[gab, 2012/09/07 20:56:44]

Done.

     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 372 matching lines @@
          installer_state.operation() == InstallerState::UNINSTALL);
   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 =

[dominich, 2012/09/06 17:26:12]

shorter:

product_state = machine_state.GetProductState(
    system_install, BrowserDistribution::CHROME_BINARIES);

2+ other places in this file.

[gab, 2012/09/07 20:56:44]

Done.

       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 =

[dominich, 2012/09/06 17:26:12]

this _should_ fit on one line.

1 other place in this file.

[gab, 2012/09/07 20:56:44]

Done.

         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);
     if (product != NULL && installer_state.is_multi_install())
       have_chrome_binaries = true;
   }
@@ skipping 188 matching lines @@
     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