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..0bba37f42be504e2fc7c5af312fe60bd381e9cc6 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -1226,6 +1226,14 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_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;
   }