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

chrome/installer/setup/setup_main.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.
 
 #include <windows.h>
 #include <msi.h>
 #include <shellapi.h>
 #include <shlobj.h>
 
 #include "base/at_exit.h"
@@ skipping 1152 matching lines @@
     }
     installer::InstallStatus tmp = installer::UNKNOWN_STATUS;
     const Product* chrome_install =
         installer_state->FindProduct(BrowserDistribution::CHROME_BROWSER);
     DCHECK(chrome_install);
     if (chrome_install) {
       installer::DeleteChromeRegistrationKeys(chrome_install->distribution(),
           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;
     base::StringToInt(cmd_line.GetSwitchValueNative(
         installer::switches::kInactiveUserToast), &flavor);
     std::string experiment_group =
         cmd_line.GetSwitchValueASCII(installer::switches::kExperimentGroup);
     DCHECK_NE(-1, flavor);
     if (flavor == -1) {
       *exit_code = installer::UNKNOWN_STATUS;
@@ skipping 316 matching lines @@
   if (!(installer_state.is_msi() && is_uninstall))
     // Note that we allow the status installer::UNINSTALL_REQUIRES_REBOOT
     // to pass through, since this is only returned on uninstall which is
     // never invoked directly by Google Update.
     return_code = InstallUtil::GetInstallReturnCode(install_status);
 
   VLOG(1) << "Installation complete, returning: " << return_code;
 
   return return_code;
 }