chrome/installer/setup/install_worker.cc - Issue 10912096: C++ reability review for gab.

Unified Diff: chrome/installer/setup/install_worker.cc

Issue 10912096: C++ reability review for gab. (Closed) Base URL: svn://svn.chromium.org/chrome/trunk/src

Patch Set: Fix up last backward if else block found with regex('if \(.* != .*\) \{(\n[^}]*)*\} else') Created 8 years, 3 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

Index: chrome/installer/setup/install_worker.cc

diff --git a/chrome/installer/setup/install_worker.cc b/chrome/installer/setup/install_worker.cc

index 4ba0e870b46d49918cc7fc96787490ae1984da23..e86341d5fb1f91df7c65e02644ae85523c88b670 100644

--- a/chrome/installer/setup/install_worker.cc

+++ b/chrome/installer/setup/install_worker.cc

@@ -5,6 +5,8 @@

// 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>

@@ -88,6 +90,7 @@ void GetIELowRightsElevationPolicyKeyPath(ElevationPolicyId policy,

// 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,

@@ -170,8 +173,8 @@ void AddInstallerCopyTasks(const InstallerState& installer_state,

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());

@@ -180,19 +183,19 @@ void AddInstallAppCommandWorkItems(const InstallerState& installer_state,

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");

}

@@ -202,18 +205,19 @@ void AddProductSpecificWorkItems(const InstallationState& original_state,

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);

}

@@ -265,28 +269,30 @@ void AddDeleteUninstallShortcutsForMSIWorkItems(

}

+// 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);

}

@@ -370,9 +376,8 @@ void AddChromeWorkItems(const InstallationState& original_state,

// 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(),

@@ -382,8 +387,8 @@ void AddChromeWorkItems(const InstallationState& original_state,

// 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.

@@ -423,16 +428,12 @@ bool ProbeCommandExecuteCallback(const string16& command_execute_id,

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();

@@ -442,9 +443,8 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,

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;

@@ -453,14 +453,12 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,

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;

}

@@ -487,9 +485,8 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,

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 {

@@ -498,8 +495,8 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,

// 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());

}

@@ -526,9 +523,9 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,

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");

}

@@ -541,8 +538,8 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,

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);

@@ -715,8 +712,8 @@ void AddOemInstallWorkItems(const InstallationState& original_state,

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,

@@ -750,12 +747,14 @@ void AddEulaAcceptedWorkItems(const InstallationState& original_state,

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);

@@ -769,8 +768,8 @@ void AddEulaAcceptedWorkItems(const InstallationState& original_state,

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,

@@ -895,15 +894,6 @@ void AddUsageStatsWorkItems(const InstallationState& original_state,

}

-// 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,

@@ -947,7 +937,7 @@ bool AppendPostInstallTasks(const InstallerState& installer_state,

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);

@@ -1042,8 +1032,8 @@ void AddInstallWorkItems(const InstallationState& original_state,

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);

@@ -1062,8 +1052,8 @@ void AddInstallWorkItems(const InstallationState& original_state,

archive_path,

src_path,

temp_path,

- new_version,

current_version,

+ new_version,

install_list);

}

@@ -1082,17 +1072,18 @@ void AddInstallWorkItems(const InstallationState& original_state,

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.

@@ -1111,16 +1102,16 @@ void AddInstallWorkItems(const InstallationState& original_state,

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);

@@ -1189,7 +1180,7 @@ void AddChromeFrameWorkItems(const InstallationState& original_state,

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()));

@@ -1284,15 +1275,14 @@ void AddChromeFrameWorkItems(const InstallationState& original_state,

// 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?";

}

@@ -1497,13 +1487,9 @@ void RefreshElevationPolicy() {

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();

@@ -1513,9 +1499,8 @@ void AddQuickEnableChromeFrameWorkItems(const InstallerState& installer_state,

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

@@ -1550,22 +1535,18 @@ void AddQuickEnableChromeFrameWorkItems(const InstallerState& 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);

@@ -1581,15 +1562,15 @@ void AddQuickEnableApplicationHostWorkItems(

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();

@@ -1599,12 +1580,15 @@ void AddOsUpgradeWorkItems(const InstallerState& installer_state,

.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.

@@ -1617,9 +1601,6 @@ void AddOsUpgradeWorkItems(const InstallerState& installer_state,

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");

}

« no previous file with comments | « chrome/installer/setup/install_worker.h ('k') | chrome/installer/setup/setup_main.cc » ('j') | chrome/installer/setup/setup_main.cc » ('J')