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

chrome/installer/setup/install_worker.cc

Index: chrome/installer/setup/install_worker.cc
diff --git a/chrome/installer/setup/install_worker.cc b/chrome/installer/setup/install_worker.cc
index 43ce81a1676efa5126c00999bd3bd8c73bbfaafc..6a35b0ac434a8c8e20bd7179eac1d61da28feaa4 100644
--- a/chrome/installer/setup/install_worker.cc
+++ b/chrome/installer/setup/install_worker.cc
@@ -171,7 +171,7 @@ void AddUninstallShortcutWorkItems(const InstallerState& installer_state,
       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);
@@ -186,7 +186,7 @@ void AddUninstallShortcutWorkItems(const InstallerState& installer_state,
     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,
@@ -201,7 +201,7 @@ void AddUninstallShortcutWorkItems(const InstallerState& installer_state,
                                          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,
@@ -250,10 +250,10 @@ void AddVersionKeyWorkItems(HKEY root,
                             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,
@@ -264,7 +264,7 @@ void AddVersionKeyWorkItems(HKEY root,
     // 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,
@@ -285,7 +285,7 @@ void AddInstallAppCommandWorkItems(const InstallerState& installer_state,
                                    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);
 
@@ -294,7 +294,9 @@ void AddInstallAppCommandWorkItems(const InstallerState& installer_state,
     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(),
@@ -319,6 +321,13 @@ void AddProductSpecificWorkItems(const InstallationState& original_state,
       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);
+    }
   }
 }
 
@@ -334,7 +343,7 @@ void AddOemInstallWorkItems(const InstallationState& original_state,
   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
@@ -353,7 +362,7 @@ void AddOemInstallWorkItems(const InstallationState& original_state,
     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)
@@ -383,7 +392,7 @@ void AddEulaAcceptedWorkItems(const InstallationState& original_state,
   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.
@@ -437,7 +446,7 @@ void AddGoogleUpdateWorkItems(const InstallationState& original_state,
 
   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
@@ -457,7 +466,7 @@ void AddGoogleUpdateWorkItems(const InstallationState& original_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
@@ -506,7 +515,7 @@ void AddUsageStatsWorkItems(const InstallationState& original_state,
   // 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
@@ -551,7 +560,7 @@ void AddDeleteUninstallShortcutsForMSIWorkItems(
 
   // 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);
@@ -629,7 +638,7 @@ bool AppendPostInstallTasks(const InstallerState& installer_state,
     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();
@@ -680,7 +689,7 @@ bool AppendPostInstallTasks(const InstallerState& installer_state,
     // 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,
@@ -987,7 +996,7 @@ void AddChromeFrameWorkItems(const InstallationState& original_state,
     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 =
@@ -1298,7 +1307,7 @@ 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,
@@ -1321,7 +1330,7 @@ void AddDeleteOldIELowRightsPolicyWorkItems(
     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);
 }
@@ -1335,8 +1344,8 @@ 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);
@@ -1398,7 +1407,7 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,
                                     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 ||
@@ -1479,7 +1488,7 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,
     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);
 
@@ -1490,7 +1499,9 @@ void AddGenericQuickEnableWorkItems(const InstallerState& installer_state,
                                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);
@@ -1593,4 +1604,37 @@ void AddQuickEnableApplicationHostWorkItems(
                                  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