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..a09e1d2809023374359adfa7309f19587c8dc395 100644
--- a/chrome/installer/setup/install_worker.cc
+++ b/chrome/installer/setup/install_worker.cc
@@ -480,6 +480,54 @@ void AddGoogleUpdateWorkItems(const InstallationState& original_state,
   // ClientState and/or Clients key.
 }
 
+void AddOsUpgradeWorkItems(const InstallationState& original_state,

[grt (UTC plus 2), 2012/08/21 20:32:55]

Since this does work for the browser only, I think it's cleaner to 1) add a
clause to AddProductSpecificWorkItems that calls this function, and 2) put
Chrome in the function name in some way.  This will let you get rid of the
installer_state.state_type() call, which doesn't do what you think it does.

[huangs, 2012/08/21 22:00:43]

Renaming to AddCromeBrowserOsUpgradeWorkItems().  Added "Browser" because
"AddChromeOsUpgradeWorkItems()" is confusing, owing to "ChromeOs".

I'm also moving routine to the bottom of the file.

+                           const InstallerState& installer_state,
+                           const FilePath& setup_path,
+                           const Version& new_version,
+                           WorkItemList* install_list) {
+  const HKEY root_key = installer_state.root_key();
+
+  // In Chrome Browser, add registry entries so that whenOS upgrade occurs,

[grt (UTC plus 2), 2012/08/21 20:32:55]

"whenOS" -> "when an OS"

[huangs, 2012/08/21 22:00:43]

Done.

+  // Google Update would call setup.exe with --on-os-upgrade switch, which

[grt (UTC plus 2), 2012/08/21 20:32:55]

"would" -> "will"

[huangs, 2012/08/21 22:00:43]

Done.

+  // leads to OnOsUpgrade() being called.
+  if (installer_state.state_type() == BrowserDistribution::CHROME_BROWSER) {
+    const Product* product = installer_state.FindProduct(
+        BrowserDistribution::CHROME_BROWSER);
+    DCHECK(product != NULL);
+    std::wstring browser_key(product->distribution()->GetVersionKey());
+    browser_key.append(1, L'\\').append(google_update::kRegCommandsKey)
+        .append(1, L'\\').append(kCmdOnOsUpgrade);
+
+    if (installer_state.operation() != InstallerState::UNINSTALL) {
+      FilePath installer_path(installer_state
+          .GetInstallerDirectory(new_version).Append(setup_path.BaseName()));
+      CommandLine command(installer_path);
+      command.AppendSwitch(installer::switches::kOnOsUpgrade);

[grt (UTC plus 2), 2012/08/21 20:32:55]

my gut tells me that you want to add kMultiInstall to the command-line when this
is a multi-install of Chrome.

[huangs, 2012/08/21 22:00:43]

Done.

+      if (installer_state.verbose_logging())
+        command.AppendSwitch(installer::switches::kVerboseLogging);
+
+      install_list->set_log_message("adding OS upgrade command");
+
+      install_list->AddCreateRegKeyWorkItem(root_key, browser_key)
+          ->set_log_message("creating AppCommand command registry key");

[grt (UTC plus 2), 2012/08/21 20:32:55]

does this use of set_log_message add useful meaning to the logs?  i don't think
this is standard practice in the installer.  should it be?

[huangs, 2012/08/21 22:00:43]

This was modelled after
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/installer/util/app_...
which I just realized is  out-of-date, so I'll remove redundant comments.

+      install_list->AddSetRegValueWorkItem(root_key, browser_key,
+                                           google_update::kRegCommandLineField,
+                                           command.GetCommandLineString(),
+                                           true)
+          ->set_log_message("setting AppCommand CommandLine registry value");
+      install_list->AddSetRegValueWorkItem(
+          root_key, browser_key, google_update::kRegAutoRunOnOSUpgrade,
+          (DWORD) 1, true)
+          ->set_log_message("setting AutoRunOnOSUpgrade registry value");
+
+    } else {
+      install_list->AddDeleteRegKeyWorkItem(root_key,

[grt (UTC plus 2), 2012/08/21 20:32:55]

is this necessary?  the whole version key is deleted on uninstall, right?

[huangs, 2012/08/21 22:00:43]

I'm following the general style of the file, where add/remove come in pairs
(remove is idempotent anyway).  Will remove if you still don't like that.  :)

+                                            browser_key)->set_log_message(
+          "Removing OS upgrade command");
+    }
+  }
+}
+
 void AddUsageStatsWorkItems(const InstallationState& original_state,
                             const InstallerState& installer_state,
                             WorkItemList* install_list) {
@@ -924,6 +972,9 @@ void AddInstallWorkItems(const InstallationState& original_state,
   // Copy over brand, usagestats, and other values.
   AddGoogleUpdateWorkItems(original_state, installer_state, install_list);
 
+  AddOsUpgradeWorkItems(original_state, installer_state, setup_path,
+                        new_version, install_list);
+
   AddQuickEnableApplicationHostWorkItems(installer_state, original_state,
                                          &setup_path, &new_version,
                                          install_list);