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

chrome/installer/setup/setup_main.cc

Index: chrome/installer/setup/setup_main.cc
diff --git a/chrome/installer/setup/setup_main.cc b/chrome/installer/setup/setup_main.cc
index 139b8274e9eb5c81aba8e38593fc0d1b12a05436..7377156260a1bb26e2967aa8b35e4b61f72d790b 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -1170,6 +1170,15 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
           HKEY_LOCAL_MACHINE, suffix, installer_state->target_path(), &tmp);
     }
     *exit_code = tmp;
+  } else if (cmd_line.HasSwitch(installer::switches::kOnOsUpgrade)) {
+    handled = false;

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

i think it's simpler to only set |handled| to false for the one case where the
switch isn't handled rather than making it false here and true later on.  so,
remove this.

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

Done.

+    if (cmd_line.HasSwitch(installer::switches::kChrome)) {

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

--chrome won't be on the command-line in all cases.  the right thing to do is to
get the Product* for chrome via
installer_state->FindProduct(BrowserDistribution::CHROME_BROWSER).  if that's
non-null, then call OnOsUpgrade.  this will simplify OnOsUpgrade, as you won't
need the FindProduct there (you can pass it in from here).

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

Done.

+      if (installer::OnOsUpgrade(original_state, *installer_state,

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

i think gab, you, and i need to discuss this in person.  i don't like that this
very browser-specific function has a general name.  i maintain that it should
have Chrome Browser in the name in some way.  it only applies to Google Chrome
(not Chromium), and it does things that only have meaning for the browser.  if
another product needs to do os upgrade work, then that work probably belongs in
its own function.

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

this needs to be:
  *exit_code = !installer::OnOsUpgrade(....)
so that the code is non-zero on failure.

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

How does this not apply to Chromium?

I agree it only applies to browsers for now though... I don't feel strongly on
naming, but we tended towards not putting "Chrome" in the function names before
since it applies just as well to Chromium.

I also feel that "BrowserOnOsUpgrade" sounds weird...

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

Renamed to HandleOsUpgradeForBrowser(), per discussion.

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

On failure, then exit_code == 1, which will be returned to Windows!  I think we
should be more explicit, and set it to
*exit_code = installer::UNKNOWN_STATUS;
on failure.  Returning 0 if successful (also hard-coded in
InstallUtil::GetInstallReturnCode()).

+                                 cmd_line.GetProgram())) {
+        *exit_code = 0;  // Successful.
+      }
+      handled = true;

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

and remove this

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

Done.

+    }

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

and change this to:
} else {
  handled = false;
}

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

Hmmmm, |handled| should not be touched by the handlers from what I see here.
|handled| == false is this method's way to know that none of the special
command-line flags were present. I don't think that us failing to handle the
--on-os-upgrade flag when its present means we should keep going with a full
regular install instead...

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

@gab's right.  do what he says.

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

Done.

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

Ah.  Replacing this with
*exit_code = installer::UNKNOWN_STATUS;
like elsewhere in the code.

   } else if (cmd_line.HasSwitch(installer::switches::kInactiveUserToast)) {
     // Launch the inactive user toast experiment.
     int flavor = -1;