Ensuring Google Update at user-level is installed alongside App Host, for the quick-enable App Host…

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 c101bb5f6c9b8a211c41538a8b3149921406e498..65f625fb17ac5f9a82cd60f49fb1e0d45bf97324 100644
--- a/chrome/installer/setup/setup_main.cc
+++ b/chrome/installer/setup/setup_main.cc
@@ -39,6 +39,7 @@
 #include "chrome/installer/util/delete_tree_work_item.h"
 #include "chrome/installer/util/google_update_constants.h"
 #include "chrome/installer/util/google_update_settings.h"
+#include "chrome/installer/util/google_update_util.h"
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/html_dialog.h"
 #include "chrome/installer/util/install_util.h"
@@ -67,7 +68,6 @@ using installer::MasterPreferences;
 const wchar_t kChromePipeName[] = L"\\\\.\\pipe\\ChromeCrashServices";
 const wchar_t kGoogleUpdatePipeName[] = L"\\\\.\\pipe\\GoogleCrashServices\\";
 const wchar_t kSystemPrincipalSid[] = L"S-1-5-18";
-const int kGoogleUpdateTimeoutMs = 20 * 1000;
 
 const MINIDUMP_TYPE kLargerDumpType = static_cast<MINIDUMP_TYPE>(
     MiniDumpWithProcessThreadData |  // Get PEB and TEB.
@@ -478,6 +478,7 @@ bool CheckMultiInstallConditions(const InstallationState& original_state,
 // satisfied, the result is written to the registry (via WriteInstallerResult),
 // |status| is set appropriately, and false is returned.
 bool CheckAppHostPreconditions(const InstallationState& original_state,
+                               const CommandLine& cmd_line,

[erikwright (departed), 2012/09/21 13:11:11]

Rather than introducing the cmd_line argument here and elsewhere, I suggest
adding a property to InstallerState (::ensure_google_update_present()) and
initializing it in InstallerState::Initialize.

Then you can just check installer_state everywhere you were previously accessing
cmd_line.

I don't think it makes sense to also put it in MasterPreferences.

grt, WDYT?

[grt (UTC plus 2), 2012/09/21 14:20:33]

sgtm.  MasterPreferences would only be required if this option needs to be set
in an installerdata block for distro deals, which doesn't appear to be the case.

[huangs, 2012/09/21 18:48:43]

Done.  "Present" sounds better than "installed", so changing that globally as
well.

                                InstallerState* installer_state,
                                installer::InstallStatus* status) {
   if (installer_state->FindProduct(BrowserDistribution::CHROME_APP_HOST)) {
@@ -500,6 +501,20 @@ bool CheckAppHostPreconditions(const InstallationState& original_state,
 
   }
 
+  // --ensure-google-update-installed is for user-level only.
+  // The flag is generic, but it's primary use case involves App Host.

[grt (UTC plus 2), 2012/09/21 14:20:33]

it's -> its

[grt (UTC plus 2), 2012/09/21 14:20:33]

if it's generic, then do this check in CheckPreInstallConditions.

[huangs, 2012/09/21 18:48:43]

How did that happen?! Done.

[huangs, 2012/09/21 18:48:43]

Done.

+  // Therefore we make the check here, with the associated status.
+  if (cmd_line.HasSwitch(installer::switches::kEnsureGoogleUpdateInstalled)) {
+    if (installer_state->system_install()) {

[grt (UTC plus 2), 2012/09/21 14:20:33]

note that you'll never get here thanks to the block above.

[huangs, 2012/09/21 18:48:43]

--ensure-google-update-installed can be specified in a stand-alone fashion for
testing (but let me know if I'm missing something).

+      LOG(DFATAL) << "--" << installer::switches::kEnsureGoogleUpdateInstalled
+                  << " is enabled for user-level only.";

[erikwright (departed), 2012/09/21 14:08:49]

enabled -> supported.

[huangs, 2012/09/21 18:48:43]

Done.

+      *status = installer::APP_HOST_REQUIRES_USER_LEVEL;
+      // No message string since there is nothing a user can do.
+      installer_state->WriteInstallerResult(*status, 0, NULL);
+      return false;
+    }
+  }
+
   return true;
 }
 
@@ -513,9 +528,11 @@ bool CheckAppHostPreconditions(const InstallationState& original_state,
 // the registry (via WriteInstallerResult), |status| is set appropriately, and
 // false is returned.
 bool CheckPreInstallConditions(const InstallationState& original_state,
+                               const CommandLine& cmd_line,
                                InstallerState* installer_state,
                                installer::InstallStatus* status) {
-  if (!CheckAppHostPreconditions(original_state, installer_state, status)) {
+  if (!CheckAppHostPreconditions(original_state, cmd_line, installer_state,
+                                 status)) {
     DCHECK_NE(*status, installer::UNKNOWN_STATUS);
     return false;
   }
@@ -851,6 +868,16 @@ installer::InstallStatus InstallProductsHelper(
     }
   }
 
+  // If Google Update is absent at user-level, install it using the installer
+  // from an existing system-level installation,
+  // This is for quick-enable App Host install from a system-level
+  // Chrome Binaries installation.
+  if (cmd_line.HasSwitch(installer::switches::kEnsureGoogleUpdateInstalled)) {

[grt (UTC plus 2), 2012/09/21 14:20:33]

if handling of this flag is general (not specific to the app host), i think it's
good to treat it as a best-effort thing in case someone adds this switch to a
normal "install Chrome" command line (it wouldn't be nice to blow up in that
case).  as such, what do you think of making the logic:
if (theNewSwitchIsPresent && isUserLevel) {
  if (!ensure...)
    LOG(ERROR) ...;
}
apologies for not suggesting this from the get-go.

[huangs, 2012/09/21 18:48:43]

Done.

+    DCHECK(!system_install);
+    if (!google_update::EnsureUserLevelGoogleUpdateInstalled())
+      LOG(ERROR) << "Failed to install Google Update";
+  }
+
   // Delete the master profile file if present. Note that we do not care about
   // rollback here and we schedule for deletion on reboot if the delete fails.
   // As such, we do not use DeleteTreeWorkItem.
@@ -887,7 +914,7 @@ installer::InstallStatus InstallProducts(
   // the -multifail suffix from the Google Update "ap" value.
   BrowserDistribution::GetSpecificDistribution(installer_state->state_type())->
       UpdateInstallStatus(system_install, archive_type, install_status);
-  if (CheckPreInstallConditions(original_state, installer_state,
+  if (CheckPreInstallConditions(original_state, cmd_line, installer_state,
                                 &install_status)) {
     VLOG(1) << "Installing to " << installer_state->target_path().value();
     install_status = InstallProductsHelper(
@@ -933,42 +960,6 @@ installer::InstallStatus UninstallProduct(
       remove_all, force_uninstall, cmd_line);
 }
 
-// Tell Google Update that an uninstall has taken place.  This gives it a chance
-// to uninstall itself straight away if no more products are installed on the
-// system rather than waiting for the next time the scheduled task runs.
-// Success or failure of Google Update has no bearing on the success or failure
-// of Chrome's uninstallation.
-void UninstallGoogleUpdate(bool system_install) {
-  string16 uninstall_cmd(
-      GoogleUpdateSettings::GetUninstallCommandLine(system_install));
-  if (!uninstall_cmd.empty()) {
-    base::win::ScopedHandle process;
-    LOG(INFO) << "Launching Google Update's uninstaller: " << uninstall_cmd;
-    if (base::LaunchProcess(uninstall_cmd, base::LaunchOptions(),
-                            process.Receive())) {
-      int exit_code = 0;
-      if (base::WaitForExitCodeWithTimeout(
-              process, &exit_code,
-              base::TimeDelta::FromMilliseconds(kGoogleUpdateTimeoutMs))) {
-        if (exit_code == 0) {
-          LOG(INFO) << "  normal exit.";
-        } else {
-          LOG(ERROR) << "Google Update uninstaller (" << uninstall_cmd
-                     << ") exited with code " << exit_code << ".";
-        }
-      } else {
-        // The process didn't finish in time, or GetExitCodeProcess failed.
-        LOG(ERROR) << "Google Update uninstaller (" << uninstall_cmd
-                   << ") is taking more than " << kGoogleUpdateTimeoutMs
-                   << " milliseconds to complete.";
-      }
-    } else {
-      PLOG(ERROR) << "Failed to launch Google Update uninstaller ("
-                  << uninstall_cmd << ")";
-    }
-  }
-}
-
 installer::InstallStatus UninstallProducts(
     const InstallationState& original_state,
     const InstallerState& installer_state,
@@ -1003,7 +994,10 @@ installer::InstallStatus UninstallProducts(
       install_status = prod_status;
   }
 
-  UninstallGoogleUpdate(installer_state.system_install());
+  // Tell Google Update that an uninstall has taken place.
+  // Ignoring the return value: Success or failure of Google Update

[grt (UTC plus 2), 2012/09/21 14:20:33]

Ignoring -> Ignore, Success -> success

[huangs, 2012/09/21 18:48:43]

Done.

+  // has no bearing on the success or failure of Chrome's uninstallation.
+  google_update::UninstallGoogleUpdate(installer_state.system_install());
 
   return install_status;
 }
@@ -1094,16 +1088,14 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
       GoogleUpdateSettings::SetEULAConsent(
           original_state, BrowserDistribution::GetDistribution(), true);
     }
-  } else if (cmd_line.HasSwitch(
-      installer::switches::kConfigureUserSettings)) {
+  } else if (cmd_line.HasSwitch(installer::switches::kConfigureUserSettings)) {
     DCHECK(installer_state->system_install());
     const Product* chrome_install =
         installer_state->FindProduct(BrowserDistribution::CHROME_BROWSER);
     DCHECK(chrome_install);
     // TODO(gab): Implement the new shortcut functionality here.
     LOG(ERROR) << "--configure-user-settings is not implemented.";
-  } else if (cmd_line.HasSwitch(
-      installer::switches::kRegisterChromeBrowser)) {
+  } else if (cmd_line.HasSwitch(installer::switches::kRegisterChromeBrowser)) {
     installer::InstallStatus status = installer::UNKNOWN_STATUS;
     const Product* chrome_install =
         installer_state->FindProduct(BrowserDistribution::CHROME_BROWSER);
@@ -1132,8 +1124,7 @@ bool HandleNonInstallCmdLineOptions(const InstallationState& original_state,
         suffix = cmd_line.GetSwitchValueNative(
             installer::switches::kRegisterChromeBrowserSuffix);
       }
-      if (cmd_line.HasSwitch(
-          installer::switches::kRegisterURLProtocol)) {
+      if (cmd_line.HasSwitch(installer::switches::kRegisterURLProtocol)) {
         string16 protocol = cmd_line.GetSwitchValueNative(
             installer::switches::kRegisterURLProtocol);
         // ShellUtil::RegisterChromeForProtocol performs all registration