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 1208 matching lines @@
         installer::ChromeFrameReadyModeTempOptOut(original_state,
                                                   *installer_state));
   } else if (cmd_line.HasSwitch(
                  installer::switches::kChromeFrameReadyModeEndTempOptOut)) {
     *exit_code = InstallUtil::GetInstallReturnCode(
         installer::ChromeFrameReadyModeEndTempOptOut(original_state,
                                                      *installer_state));
   } else if (cmd_line.HasSwitch(installer::switches::kChromeFrameQuickEnable)) {
     *exit_code = installer::ChromeFrameQuickEnable(original_state,
                                                    installer_state);
+  } else if (cmd_line.HasSwitch(installer::switches::kOnOsUpgrade)) {

[gab, 2012/08/28 16:08:19]

All the last switches are ChromeFrame related, I feel this belongs higher in
this "else if madness", closer to the Chrome  switches with similar goal like
installer::switches::kRegisterChromeBrowser

Conversely, please move all the other functions around to match the new
alignment here (as it seems other functions, e.g. in install_worker.cc, do
that).

Please address comments in one patch set and do a move with no changes in
another patch set (easier to see what changed in the diff).

[grt (UTC plus 2), 2012/08/28 19:35:39]

i strongly advise against changing the order of the other conditionals here. 
you'll break at least one feature if you reorder two of the clauses in
particular.  i'll leave it as an exercise to the reader to figure out which
ones.  :-)

[gab, 2012/08/29 03:23:48]

No no no! I'm not suggesting to move other calls! I'm just saying this new one
should be above (not here with Chrome Frame specific stuff). What I'm otherwise
also saying is that the alignment of the new corresponding function in
install_worker.cc and the likes should also be aligned at the same place (i.e.
w.r.t other installer:: functions called here -- as those seem to respect the
order here from what I looked at).

I feel I'm having a hard-time expressing what I mean in words here, we can talk
about it tomorrow if that's not clear.

[huangs, 2012/08/29 17:02:54]

Moved (and only moved) my code.

+    handled = false;
+    if (cmd_line.HasSwitch(installer::switches::kChrome)) {

[grt (UTC plus 2), 2012/08/28 19:35:39]

one way to make this more OO would be to add a method to the  ProductOperations
interface (you could call it HandleOSUpgrade, OnOsUpgrade, or somesuch),
implement it for ChromeBrowserOperations, and add a Product::HandleOsUpgrade
that delegated to the underlying operations implementation.  the code here,
then, could be totally generic: iterate over all products and call the new
Product method.  the registration code wouldn't change: register only for the
product Chrome, and only Chrome's will be hit.  if another product needs it in
the future, it'll be clear where to put the code.

[huangs, 2012/08/29 17:02:54]

Doing this would bring in another 11 files to the CL!  Can I do this after, as a
refactoring CL?

+      *exit_code = InstallUtil::GetInstallReturnCode(
+          installer::ChromeBrowserOnOsUpgrade(original_state, *installer_state,
+                                              cmd_line));
+      handled = true;
+    }
   } else {
     handled = false;
   }
 
   return handled;
 }
 
 bool ShowRebootDialog() {
   // Get a token for this process.
   HANDLE token;
@@ skipping 260 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;
 }