Callback flow to register Chrome and update shortcuts after OS upgrade to Windows 8

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.
 
 #include "chrome/installer/setup/install_worker.h"
 
 #include <oaidl.h>
@@ skipping 153 matching lines @@
   // means that Chrome Frame is being uninstalled, so there's no need to do any
   // looping.
   if (product.is_chrome() &&
       installer_state.operation() != InstallerState::UNINSTALL) {
     const Product* chrome_frame =
         installer_state.FindProduct(BrowserDistribution::CHROME_FRAME);
     if (chrome_frame && chrome_frame->HasOption(kOptionReadyMode))
       chrome_frame->AppendProductFlags(&uninstall_arguments);
   }
 
-  std::wstring update_state_key(browser_dist->GetStateKey());
+  string16 update_state_key(browser_dist->GetStateKey());
   install_list->AddCreateRegKeyWorkItem(reg_root, update_state_key);
   install_list->AddSetRegValueWorkItem(reg_root, update_state_key,
       installer::kUninstallStringField, installer_path.value(), true);
   install_list->AddSetRegValueWorkItem(reg_root, update_state_key,
       installer::kUninstallArgumentsField,
       uninstall_arguments.GetCommandLineString(), true);
 
   // MSI installations will manage their own uninstall shortcuts.
   if (!installer_state.is_msi() && product.ShouldCreateUninstallEntry()) {
     // We need to quote the command line for the Add/Remove Programs dialog.
     CommandLine quoted_uninstall_cmd(installer_path);
     DCHECK_EQ(quoted_uninstall_cmd.GetCommandLineString()[0], '"');
     quoted_uninstall_cmd.AppendArguments(uninstall_arguments, false);
 
-    std::wstring uninstall_reg = browser_dist->GetUninstallRegPath();
+    string16 uninstall_reg = browser_dist->GetUninstallRegPath();
     install_list->AddCreateRegKeyWorkItem(reg_root, uninstall_reg);
     install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
         installer::kUninstallDisplayNameField,
         browser_dist->GetAppShortCutName(), true);
     install_list->AddSetRegValueWorkItem(reg_root,
         uninstall_reg, installer::kUninstallStringField,
         quoted_uninstall_cmd.GetCommandLineString(), true);
     install_list->AddSetRegValueWorkItem(reg_root,
                                          uninstall_reg,
                                          L"InstallLocation",
                                          install_path.value(),
                                          true);
 
     // DisplayIcon, NoModify and NoRepair
-    std::wstring chrome_icon = ShellUtil::GetChromeIcon(
+    string16 chrome_icon = ShellUtil::GetChromeIcon(
         product.distribution(),
         install_path.Append(installer::kChromeExe).value());
     install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
                                          L"DisplayIcon", chrome_icon, true);
     install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
                                          L"NoModify", static_cast<DWORD>(1),
                                          true);
     install_list->AddSetRegValueWorkItem(reg_root, uninstall_reg,
                                          L"NoRepair", static_cast<DWORD>(1),
                                          true);
@@ skipping 28 matching lines @@
 
 // Create Version key for a product (if not already present) and sets the new
 // product version as the last step.
 void AddVersionKeyWorkItems(HKEY root,
                             BrowserDistribution* dist,
                             const Version& new_version,
                             bool add_language_identifier,
                             WorkItemList* list) {
   // Create Version key for each distribution (if not already present) and set
   // the new product version as the last step.
-  std::wstring version_key(dist->GetVersionKey());
+  string16 version_key(dist->GetVersionKey());
   list->AddCreateRegKeyWorkItem(root, version_key);
 
-  std::wstring product_name(dist->GetAppShortCutName());
+  string16 product_name(dist->GetAppShortCutName());
   list->AddSetRegValueWorkItem(root, version_key, google_update::kRegNameField,
                                product_name, true);  // overwrite name also
   list->AddSetRegValueWorkItem(root, version_key,
                                google_update::kRegOopcrashesField,
                                static_cast<DWORD>(1),
                                false);  // set during first install
   if (add_language_identifier) {
     // Write the language identifier of the current translation.  Omaha's set of
     // languages is a superset of Chrome's set of translations with this one
     // exception: what Chrome calls "en-us", Omaha calls "en".  sigh.
-    std::wstring language(GetCurrentTranslation());
+    string16 language(GetCurrentTranslation());
     if (LowerCaseEqualsASCII(language, "en-us"))
       language.resize(2);
     list->AddSetRegValueWorkItem(root, version_key,
                                  google_update::kRegLangField, language,
                                  false);  // do not overwrite language
   }
   list->AddSetRegValueWorkItem(root, version_key,
                                google_update::kRegVersionField,
                                ASCIIToWide(new_version.GetString()),
                                true);  // overwrite version
 }
 
 void AddInstallAppCommandWorkItems(const InstallerState& installer_state,
                                    const InstallationState& machine_state,
                                    const FilePath* setup_path,
                                    const Version* new_version,
                                    const Product& product,
                                    WorkItemList* work_item_list) {
   DCHECK(product.is_chrome_app_host());
 
-  std::wstring cmd_key(product.distribution()->GetVersionKey());
+  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));
     cmd_line.AppendSwitchASCII(::switches::kAppsInstallFromManifestURL, "%1");
 
-    AppCommand cmd(cmd_line.GetCommandLineString(), true, true);
+    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];
     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()) {
+      // In Chrome Browser, add registry entries so that when an OS upgrade

[grt (UTC plus 2), 2012/08/30 04:15:25]

i think this comment is stating the obvious: the line above makes it clear that
this only applies to chrome (so "In Chrome Browser..." is redundant), and
"AddOsUpgradeWorkItems" is pretty self-documenting.  i think the nugget that
Google Update will call setup.exe would be well-placed in the doc comment for
AddOsUpgradeWorkItems.

[huangs, 2012/08/30 17:13:07]

Deleted redundancy; moved comment to body of AddOsUpgradeWorkItems().

+      // occurs, Google Update will call setup.exe with --on-os-upgrade switch,
+      // which leads to ChromeBrowserOnOsUpgrade() being called.

[grt (UTC plus 2), 2012/08/30 04:15:25]

"ChromeBrowserOnOsUpgrade" -> "OnOsUpgrade"

[huangs, 2012/08/30 17:13:07]

Done => new name is HandleOsUpgradeForBrowser().

+      AddOsUpgradeWorkItems(installer_state, &setup_path, &new_version, p,
+                            list);
+    }
   }
 }
 
 // Mirror oeminstall the first time anything is installed multi.  There is no
 // need to update the value on future install/update runs since this value never
 // changes.  Note that the value is removed by Google Update after EULA
 // acceptance is processed.
 void AddOemInstallWorkItems(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();
-    std::wstring multi_key(
+    string16 multi_key(
         installer_state.multi_package_binaries_distribution()->GetStateKey());
 
     // Copy the value from Chrome unless Chrome isn't installed or being
     // installed.
     BrowserDistribution::Type source_type;
     if (installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER)) {
       source_type = BrowserDistribution::CHROME_BROWSER;
     } else if (!installer_state.products().empty()) {
       // Pick a product, any product.
       source_type = installer_state.products()[0]->distribution()->GetType();
     } else {
       // 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);
 
-    std::wstring oem_install;
+    string16 oem_install;
     if (source_product->GetOemInstall(&oem_install)) {
       VLOG(1) << "Mirroring oeminstall=\"" << oem_install << "\" from "
               << 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
 // 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();
-    std::wstring multi_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())
         continue;
@@ skipping 33 matching lines @@
                               WorkItemList* install_list) {
   // Is a multi-install product being installed or over-installed?
   if (installer_state.operation() != InstallerState::MULTI_INSTALL &&
       installer_state.operation() != InstallerState::MULTI_UPDATE) {
     VLOG(1) << "AddGoogleUpdateWorkItems noop: " << installer_state.operation();
     return;
   }
 
   const bool system_install = installer_state.system_install();
   const HKEY root_key = installer_state.root_key();
-  std::wstring multi_key(
+  string16 multi_key(
       installer_state.multi_package_binaries_distribution()->GetStateKey());
 
   // For system-level installs, make sure the ClientStateMedium key for the
   // binaries exists.
   if (system_install) {
     install_list->AddCreateRegKeyWorkItem(
         root_key,
         installer_state.multi_package_binaries_distribution()->
             GetStateMediumKey().c_str());
   }
 
   // Creating the ClientState key for binaries, if we're migrating to multi then
   // copy over Chrome's brand code if it has one. Chrome Frame currently never
   // has a brand code.
   if (installer_state.state_type() != BrowserDistribution::CHROME_BINARIES) {
     const ProductState* chrome_product_state =
         original_state.GetNonVersionedProductState(
             system_install, BrowserDistribution::CHROME_BROWSER);
 
-    const std::wstring& brand(chrome_product_state->brand());
+    const string16& brand(chrome_product_state->brand());

[grt (UTC plus 2), 2012/08/30 04:15:25]

please revert this since ProductState::brand still returns a std::wstring.  i'd
rather see the methods change before the consumers rather than the other way
around.

[gab, 2012/08/30 14:24:32]

Does it matter? I'd rather be consistent on a per-file basis.

[grt (UTC plus 2), 2012/08/30 19:10:18]

My preferred way of doing the wstring -> string16 migration is to do it
horizontally on a per-API or per-class basis rather than file by file.  In the
former case, producers and consumers are consistent.  In the latter case, you
end up with a given "thing" being a std::wstring in one place and a string16 in
another, which I feel decreases readability.

     if (!brand.empty()) {
       install_list->AddCreateRegKeyWorkItem(root_key, multi_key);
       // Write Chrome's brand code to the multi key. Never overwrite the value
       // if one is already present (although this shouldn't happen).
       install_list->AddSetRegValueWorkItem(root_key,
                                            multi_key,
                                            google_update::kRegBrandField,
                                            brand,
                                            false);
     }
@@ skipping 28 matching lines @@
     BrowserDistribution* dist = (*scan)->distribution();
     const ProductState* product_state =
         original_state.GetNonVersionedProductState(
             installer_state.system_install(), dist->GetType());
     value_found = product_state->GetUsageStats(&usagestats);
   }
 
   // If a value was found, write it in the appropriate location for the
   // binaries and remove all values from the products.
   if (value_found) {
-    std::wstring state_key(
+    string16 state_key(
         installer_state.multi_package_binaries_distribution()->GetStateKey());
     install_list->AddCreateRegKeyWorkItem(root_key, state_key);
     // Overwrite any existing value so that overinstalls (where Omaha writes a
     // new value into a product's state key) pick up the correct value.
     install_list->AddSetRegValueWorkItem(root_key, state_key,
                                          google_update::kRegUsageStatsField,
                                          usagestats, true);
 
     for (Products::const_iterator scan = products.begin(), end = products.end();
          scan != end; ++scan) {
@@ skipping 24 matching lines @@
 void AddDeleteUninstallShortcutsForMSIWorkItems(
     const InstallerState& installer_state,
     const Product& product,
     const FilePath& temp_path,
     WorkItemList* work_item_list) {
   DCHECK(installer_state.is_msi())
       << "This must only be called for MSI installations!";
 
   // First attempt to delete the old installation's ARP dialog entry.
   HKEY reg_root = installer_state.root_key();
-  std::wstring uninstall_reg(product.distribution()->GetUninstallRegPath());
+  string16 uninstall_reg(product.distribution()->GetUninstallRegPath());
 
   WorkItem* delete_reg_key = work_item_list->AddDeleteRegKeyWorkItem(
       reg_root, uninstall_reg);
   delete_reg_key->set_ignore_failure(true);
 
   // Then attempt to delete the old installation's start menu shortcut.
   FilePath uninstall_link;
   if (installer_state.system_install()) {
     PathService::Get(base::DIR_COMMON_START_MENU, &uninstall_link);
   } else {
@@ skipping 57 matching lines @@
         .Append(setup_path.BaseName()));
 
     CommandLine rename(installer_path);
     rename.AppendSwitch(switches::kRenameChromeExe);
     if (installer_state.system_install())
       rename.AppendSwitch(switches::kSystemLevel);
 
     if (installer_state.verbose_logging())
       rename.AppendSwitch(switches::kVerboseLogging);
 
-    std::wstring version_key;
+    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) {
         in_use_update_work_items->AddSetRegValueWorkItem(root, version_key,
             google_update::kRegOldVersionField,
             ASCIIToWide(current_version->GetString()), true);
       }
       if (critical_version.IsValid()) {
@@ skipping 30 matching lines @@
   // Append work items that will be executed if this was NOT an in-use update.
   {
     scoped_ptr<WorkItemList> regular_update_work_items(
         WorkItem::CreateConditionalWorkItemList(
             new Not(new ConditionRunIfFileExists(new_chrome_exe))));
     regular_update_work_items->set_log_message("RegularUpdateWorkItemList");
 
     // Since this was not an in-use-update, delete 'opv', 'cpv', and 'cmd' keys.
     for (size_t i = 0; i < products.size(); ++i) {
       BrowserDistribution* dist = products[i]->distribution();
-      std::wstring version_key(dist->GetVersionKey());
+      string16 version_key(dist->GetVersionKey());
       regular_update_work_items->AddDeleteRegValueWorkItem(root, version_key,
           google_update::kRegOldVersionField);
       regular_update_work_items->AddDeleteRegValueWorkItem(root, version_key,
           google_update::kRegCriticalVersionField);
       regular_update_work_items->AddDeleteRegValueWorkItem(root, version_key,
           google_update::kRegRenameCmdField);
     }
 
     if (installer_state.FindProduct(BrowserDistribution::CHROME_FRAME)) {
       AddDeleteOldIELowRightsPolicyWorkItems(installer_state,
@@ skipping 286 matching lines @@
                              const FilePath& setup_path,
                              const Version& new_version,
                              const Product& product,
                              WorkItemList* list) {
   DCHECK(product.is_chrome_frame());
   if (!installer_state.is_multi_install()) {
     VLOG(1) << "Not adding GCF specific work items for single install.";
     return;
   }
 
-  std::wstring version_key(product.distribution()->GetVersionKey());
+  string16 version_key(product.distribution()->GetVersionKey());
   bool ready_mode = product.HasOption(kOptionReadyMode);
   HKEY root = installer_state.root_key();
   const bool is_install =
       (installer_state.operation() != InstallerState::UNINSTALL);
   bool update_chrome_uninstall_command = false;
   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
@@ skipping 290 matching lines @@
 // this is done only as a convenience; there is no need for the GUID of the Low
 // Rights policies to match the ChromeFrame class's GUID.  Hence, it is safe to
 // use this completely unrelated GUID for the "old" policies.
 const wchar_t kIELowRightsPolicyOldGuid[] =
     L"{6C288DD7-76FB-4721-B628-56FAC252E199}";
 
 const wchar_t kElevationPolicyKeyPath[] =
     L"SOFTWARE\\Microsoft\\Internet Explorer\\Low Rights\\ElevationPolicy\\";
 
 void GetIELowRightsElevationPolicyKeyPath(ElevationPolicyId policy,
-                                          std::wstring* key_path) {
+                                          string16* key_path) {
   DCHECK(policy == CURRENT_ELEVATION_POLICY || policy == OLD_ELEVATION_POLICY);
 
   key_path->assign(kElevationPolicyKeyPath,
                    arraysize(kElevationPolicyKeyPath) - 1);
   if (policy == CURRENT_ELEVATION_POLICY) {
     wchar_t cf_clsid[64];
     int len = StringFromGUID2(__uuidof(ChromeFrame), &cf_clsid[0],
                               arraysize(cf_clsid));
     key_path->append(&cf_clsid[0], len - 1);
   } else {
     key_path->append(kIELowRightsPolicyOldGuid,
                      arraysize(kIELowRightsPolicyOldGuid)- 1);
   }
 }
 
 }  // namespace
 
 void AddDeleteOldIELowRightsPolicyWorkItems(
     const InstallerState& installer_state,
     WorkItemList* install_list) {
   DCHECK(install_list);
 
-  std::wstring key_path;
+  string16 key_path;
   GetIELowRightsElevationPolicyKeyPath(OLD_ELEVATION_POLICY, &key_path);
   install_list->AddDeleteRegKeyWorkItem(installer_state.root_key(), key_path);
 }
 
 // Adds work items to copy the chrome_launcher IE low rights elevation policy
 // from the primary policy GUID to the "old" policy GUID.  Take care not to
 // perform the copy if there is already an old policy present, as the ones under
 // the main kElevationPolicyGuid would then correspond to an intermediate
 // version (current_version < pv < new_version).
 void AddCopyIELowRightsPolicyWorkItems(const InstallerState& installer_state,
                                        WorkItemList* install_list) {
   DCHECK(install_list);
 
-  std::wstring current_key_path;
-  std::wstring old_key_path;
+  string16 current_key_path;
+  string16 old_key_path;
 
   GetIELowRightsElevationPolicyKeyPath(CURRENT_ELEVATION_POLICY,
                                        &current_key_path);
   GetIELowRightsElevationPolicyKeyPath(OLD_ELEVATION_POLICY, &old_key_path);
   // Do not clobber existing old policies.
   install_list->AddCopyRegKeyWorkItem(installer_state.root_key(),
                                       current_key_path, old_key_path,
                                       WorkItem::IF_NOT_PRESENT);
 }
 
@@ skipping 41 matching lines @@
   }
 }
 
 void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,
                                     const InstallationState& machine_state,
                                     const FilePath* setup_path,
                                     const Version* new_version,
                                     WorkItemList* work_item_list,
                                     bool have_child_product,
                                     const CommandLine& child_product_switches,
-                                    const std::wstring& command_id) {
+                                    const string16& command_id) {
   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_binaries = false;
 
   // STEP 1: Figure out the state of the machine before the operation.
@@ skipping 60 matching lines @@
               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);
-    std::wstring cmd_key(binaries->GetVersionKey());
+    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(), true, true);
+      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");
     }
   }
 }
 
@@ skipping 82 matching lines @@
   AddGenericQuickEnableWorkItems(installer_state,
                                  machine_state,
                                  setup_path,
                                  new_version,
                                  work_item_list,
                                  false,  // have_child_product
                                  cmd_line,
                                  kCmdQuickEnableApplicationHost);
 }
 
+void AddOsUpgradeWorkItems(const InstallerState& installer_state,
+                           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());

[gab, 2012/08/30 14:24:32]

Why initialize and then use append?
I would prefer:
const string16 cmd_key(product.distribution()->GetVersionKey()
    .append(...)
    .append(...)
     ....);

[huangs, 2012/08/30 17:13:07]

Per our discussion, not changing.

+  cmd_key.append(1, FilePath::kSeparators[0])
+      .append(google_update::kRegCommandsKey)
+      .append(1, FilePath::kSeparators[0]).append(kCmdOnOsUpgrade);

[grt (UTC plus 2), 2012/08/30 04:15:25]

nit: move this last append to its own line

[huangs, 2012/08/30 17:13:07]

Done.

+
+  if (installer_state.operation() != InstallerState::UNINSTALL) {
+    CommandLine cmd_line(installer_state
+        .GetInstallerDirectory(*new_version).Append(setup_path->BaseName()));

[gab, 2012/08/30 14:24:32]

s/setup_path->BaseName()/installer::kSetupExe

and then you can remove |setup_path| as an argument.

[grt (UTC plus 2), 2012/08/30 17:06:08]

Please don't.  BaseName() of the exe is what's used to put setup.exe into the
installation, so it is the canonical name that should be used.  BaseName() is
the convention for this pupose.  This makes it much easier if the names should
ever change in the future.

[huangs, 2012/08/30 17:13:07]

Done.

[gab, 2012/08/30 17:15:50]

Changing the name in the future would be the craziest thing on earth imo, but ok
:)!

+    // 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);
+    if (installer_state.verbose_logging())
+      cmd_line.AppendSwitch(installer::switches::kVerboseLogging);

[grt (UTC plus 2), 2012/08/30 04:15:25]

i think this should be added unconditionally, at least for now, so that we get
useful data from the field when bugs are filed.

[huangs, 2012/08/30 17:13:07]

Done.

+
+    install_list->set_log_message("Adding OS upgrade command");
+    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->set_log_message("Removing OS upgrade command");

[grt (UTC plus 2), 2012/08/30 04:15:25]

i think you want to set the message for the item added on the line below, not on
the list itself.

[huangs, 2012/08/30 17:13:07]

Done.

+    install_list->AddDeleteRegKeyWorkItem(root_key, cmd_key);
+  }
+}
+
 }  // namespace installer