[Fixit-Dec-2012] Auto-launch system-level Chrome post user-level Chrome self-destruction.

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 fef8e4cd4ddf4f8aa21a4d365dcb19cd068ec420..b649787dcafa2b86778deb1cb9d2ea07be64e500 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -13,6 +13,7 @@
 #include "base/file_util.h"
 #include "base/file_version_info.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/path_service.h"
 #include "base/process_util.h"
 #include "base/string16.h"
@@ -28,6 +29,7 @@
 #include "base/win/win_util.h"
 #include "base/win/windows_version.h"
 #include "breakpad/src/client/windows/handler/exception_handler.h"
+#include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_switches.h"
 #include "chrome/installer/setup/chrome_frame_quick_enable.h"
 #include "chrome/installer/setup/chrome_frame_ready_mode.h"
@@ -1004,12 +1006,41 @@ installer::InstallStatus UninstallProducts(
     const CommandLine& cmd_line) {
   const Products& products = installer_state.products();
 
+  // Decide whether Active Setup should be triggered and/or system-level Chrome
+  // should be launched post-uninstall. This needs to be done outside the
+  // UninstallProduct calls as some of them might terminate the processes
+  // launched by a previous one otherwise...
+  bool trigger_active_setup = false;
+  // System-level Chrome will be launched via this command if it gets set below.
+  scoped_ptr<CommandLine> system_level_cmd;

[grt (UTC plus 2), 2013/01/02 17:59:33]

It seems more straightforward to use:
  CommandLine system_level_cmd(CommandLine::NO_PROGRAM);
here,
       SetProgram(system_exe_path);
on line 1030, and
     if (!system_level_cmd.GetProgram().empty())
on line 1073, in which case scoped_ptr.h isn't needed.

[gab, 2013/01/02 21:15:48]

Done.

+
   if (installer_state.FindProduct(BrowserDistribution::CHROME_BROWSER)) {
     // InstallerState::Initialize always puts Chrome first, and we rely on that
     // here for this reason: if Chrome is in-use, the user will be prompted to
     // confirm uninstallation.  Upon cancel, we should not continue with the
     // other products.
     DCHECK(products[0]->is_chrome());
+
+    if (cmd_line.HasSwitch(installer::switches::kSelfDestruct)) {

[grt (UTC plus 2), 2013/01/02 17:59:33]

&& !system_level() since this shouldn't be done if someone accidentally puts
--self-destruct on a system-level uninstall command line?

[gab, 2013/01/02 21:15:48]

Sure.

+      BrowserDistribution* dist = BrowserDistribution::GetSpecificDistribution(
+          BrowserDistribution::CHROME_BROWSER);
+      const FilePath system_exe_path(
+          installer::GetChromeInstallPath(true, dist)
+              .Append(installer::kChromeExe));
+      system_level_cmd.reset(new CommandLine(system_exe_path));
+
+      FilePath first_run_sentinel;
+      InstallUtil::GetSentinelFilePath(
+          chrome::kFirstRunSentinel, false, products[0]->distribution(),

[grt (UTC plus 2), 2013/01/02 17:59:33]

isn't products[0]->distribution() the same as |dist|?

[gab, 2013/01/02 21:15:48]

Indeed :)!

+          &first_run_sentinel);
+      if (file_util::PathExists(first_run_sentinel)) {
+        // If the Chrome being self-destructed has already undergone First Run,
+        // trigger Active Setup and make sure the system-level Chrome doesn't go
+        // through first run.

[grt (UTC plus 2), 2013/01/02 17:59:33]

is there a difference between using --no-first-run and simply creating the
system-level beacon here?

[gab, 2013/01/02 21:15:48]

Hmmm, well arguably dropping the beacon here could fail (i.e. write error) where
as passing --no-first-run can't really fail...

+        trigger_active_setup = true;
+        system_level_cmd->AppendSwitch(::switches::kNoFirstRun);

[grt (UTC plus 2), 2013/01/02 17:59:33]

nit: remove the :: in front of "switches" unless it's actually necessary.

[gab, 2013/01/02 21:15:48]

It is necessary since installer::switches also exists and this code lives in the
installer:: namespace.
kNoFirstRun is in switches:: not installer::switches::.

+      }
+    }
   }
   if (installer_state.FindProduct(BrowserDistribution::CHROME_BINARIES)) {
     // Chrome Binaries should be last; if something else is cancelled, they
@@ -1035,6 +1066,14 @@ installer::InstallStatus UninstallProducts(
   installer::CleanUpInstallationDirectoryAfterUninstall(
       original_state, installer_state, cmd_line, &install_status);
 
+  if (install_status != installer::UNINSTALL_CANCELLED) {
+    if (trigger_active_setup)

[grt (UTC plus 2), 2013/01/02 17:59:33]

install_status may be any of: (CHROME_NOT_INSTALLED, UNINSTALL_SUCCESSFUL,
UNINSTALL_FAILED, UNINSTALL_REQUIRES_REBOOT). should active setup and/or the
handoff happen for all of those cases?

[gab, 2013/01/02 21:15:48]

In fact I think it was wrong to base this off of "install_status !=
installer::UNINSTALL_CANCELLED" (had copied that logic from the for loop
condition -- and UNINSTALL_CANCELLED is only set by the user prompt which isn't
even shown with --force-uninstall anyways...).

I think that we should always launch Active Setup and the system-level Chrome no
matter the uninstall result.

i.e., I think it's a better user experience to keep getting the self-destruct
prompt in case of repetitive uninstall failures, but at least get system-level
Chrome to launch, then it is to only get the prompt with no Chrome launching...

I could wrap all of this in an if block that also checks for the presence of
kSelfDestruct; that would be redundant, but perhaps more readable..?

+      InstallUtil::TriggerActiveSetupCommand();
+
+    if (system_level_cmd.get())
+      base::LaunchProcess(*system_level_cmd, base::LaunchOptions(), NULL);
+  }
+
   // Tell Google Update that an uninstall has taken place.
   // Ignore the return value: success or failure of Google Update
   // has no bearing on the success or failure of Chrome's uninstallation.
@@ -1068,10 +1107,11 @@ installer::InstallStatus ShowEULADialog(const string16& inner_frame) {
 
 // Creates the sentinel indicating that the EULA was required and has been
 // accepted.
-bool CreateEULASentinel(BrowserDistribution* dist) {
+bool CreateEULASentinel(bool system_install,

[grt (UTC plus 2), 2013/01/02 17:59:33]

The EULA only applies for system-level installs, so you can remove
|system_install| and pass |true| directly to GetSentinelFilePath.

[gab, 2013/01/02 21:15:48]

Done.

+                        BrowserDistribution* dist) {
   FilePath eula_sentinel;
-  if (!InstallUtil::GetSentinelFilePath(installer::kEULASentinelFile,
-                                        dist, &eula_sentinel)) {
+  if (!InstallUtil::GetSentinelFilePath(
+          installer::kEULASentinelFile, system_install, dist, &eula_sentinel)) {
     return false;
   }
 
@@ -1241,7 +1281,8 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
     if (installer::EULA_REJECTED != *exit_code) {
       if (GoogleUpdateSettings::SetEULAConsent(
               original_state, BrowserDistribution::GetDistribution(), true)) {
-        CreateEULASentinel(BrowserDistribution::GetDistribution());
+        CreateEULASentinel(installer_state->system_install(),

[grt (UTC plus 2), 2013/01/02 17:59:33]

FYI: --system-level isn't passed to setup.exe in this case, so
installer_state->system_install() cannot be used. this isn't a problem, however;
see comment on line 1110.

[gab, 2013/01/02 21:15:48]

Done.

+                           BrowserDistribution::GetDistribution());
       }
       // For a metro-originated launch, we now need to launch back into metro.
       if (cmd_line.HasSwitch(installer::switches::kShowEulaForMetro))