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

chrome/installer/util/google_update_util.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 "chrome/installer/util/google_update_util.h"
+
+#include "base/command_line.h"
+#include "base/file_path.h"
+#include "base/file_util.h"
+#include "base/logging.h"
+#include "base/process_util.h"
+#include "base/string16.h"
+#include "base/win/registry.h"
+#include "base/win/scoped_handle.h"
+#include "chrome/installer/launcher_support/chrome_launcher_support.h"
+#include "chrome/installer/util/google_update_constants.h"
+#include "chrome/installer/util/google_update_settings.h"
+
+using base::win::RegKey;
+
+namespace google_update {
+
+namespace {
+
+const int kGoogleUpdateTimeoutMs = 20 * 1000;
+
+// Returns true if and only if Google Update is installed at the given level.
+bool IsGoogleUpdateInstalled(bool system_install) {
+  // Using the existence of version key in the registry to decide.
+  return GoogleUpdateSettings::GetGoogleUpdateVersion(system_install).IsValid();
+}
+
+// Returns GoogleUpdateSetup.exe's executable path at |system_level|,
+// or an empty path if none is found.
+FilePath GetGoogleUpdateSetupExe(bool system_install) {
+  const HKEY root_key = system_install ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER;
+  RegKey update_key;
+
+  if (update_key.Open(root_key, kRegPathGoogleUpdate, KEY_QUERY_VALUE) ==
+          ERROR_SUCCESS) {
+    string16 path_str;
+    string16 version_str;
+    if ((update_key.ReadValue(kRegPathField, &path_str) == ERROR_SUCCESS) &&
+        (update_key.ReadValue(kRegGoogleUpdateVersion, &version_str) ==
+             ERROR_SUCCESS)) {
+      FilePath google_update_setup(FilePath(path_str).DirName().
+                                   Append(version_str).
+                                   Append(kGoogleUpdateSetupExe));
+      if (file_util::PathExists(google_update_setup)) {

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

i suggest you don't bother with this check.  it's racy.  just return the path
here and let CreateProcess fail if it fails.

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

Done.

+        return google_update_setup;
+      } else {
+        LOG(ERROR) << "Unexpectedly missing " << kGoogleUpdateSetupExe;

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

log google_update_setup instead (so full path is included).

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

Moot, as check is removed.

+      }
+    }
+  }
+  return FilePath();
+}
+
+// Returns command line to install Google Update at user-level if Google Update
+// is already installed at system-level,or an empty command line if none found.
+// This is not in GoogleUpdateSettings because we build the command line,

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

Remove "This is not ..."

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

Done.

+// instead of just reading it from the registry.
+CommandLine GetUserLevelGoogleUpdateInstallCommandLine() {
+  CommandLine cmd(CommandLine::NO_PROGRAM);
+  FilePath google_update_setup(GetGoogleUpdateSetupExe(true));  // system-level.
+  if (!google_update_setup.empty()) {
+    cmd.SetProgram(google_update_setup);
+    // Appends parameter "/install runtime=true&needsadmin=false /silent"
+    // Constants are found in code.google.com/p/omaha/common/const_cmd_line.h.
+    cmd.AppendArg("/install");
+    // The "&" can be used in base::LaunchProcess() without quotation
+    // (this is problematic only if run from command prompt).
+    cmd.AppendArg("runtime=true&needsadmin=false");
+    cmd.AppendArg("/silent");
+  }
+  return cmd;
+}
+
+bool ExecuteGoogleUpdateCommand(const string16& cmd_string,

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

please add a doc comment

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

Moot: Routine inlined.

+                                const string16& cmd_name,
+                                base::win::ScopedHandle* process) {
+  bool success = false;
+  if (cmd_string.empty()) {  // Command not rendered.

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

remove this whole block and let an empty cmd_string percolate down through
LaunchProcess

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

Moot: Routine inlined.

+    LOG(ERROR) << "Failed to find " << cmd_name;
+  } else {
+    LOG(INFO) << "Launching " << cmd_name << ": " << cmd_string;

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

just log cmd_string instead of cmd_name here.

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

Done.

+    success = base::LaunchProcess(cmd_string, base::LaunchOptions(),
+                                  process->Receive());
+    if (!success) {
+      PLOG(ERROR) << "Failed to launch " << cmd_name << " (" << cmd_string

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

no need to include cmd_name.

+                  << ").";

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

remove the period at the end here since PLOG will add extra stuff to the log
message.

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

Done.

+    }
+  }
+  return success;
+}
+
+bool WaitForGoogleUpdateCommand(const string16& cmd_string,
+                                const string16& cmd_name,
+                                const base::win::ScopedHandle& process) {

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

Define |process| as a plain-old HANDLE. There is an implicit conversion
available.

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

Moot: Function inlined.  Note extra complexity with ProcessHandle.

+  bool success = false;
+  int exit_code = 0;
+  if (base::WaitForExitCodeWithTimeout(
+          process, &exit_code,

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

please move these to the previous line to conserve vertical space.

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

Done.

+          base::TimeDelta::FromMilliseconds(kGoogleUpdateTimeoutMs))) {
+    if (exit_code == 0) {
+      LOG(INFO) << "  normal exit.";

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

i should never have put this there, please remove it.

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

Done.

+      success = true;
+    } else {
+      PLOG(ERROR) << cmd_name << " (" << cmd_string << ") exited with code "

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

no need for cmd_name here.

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

PLOG -> LOG

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

Done.

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

Done.

+                  << exit_code << ".";
+    }
+  } else {
+    // The process didn't finish in time, or GetExitCodeProcess failed.
+    LOG(ERROR) << cmd_name << " (" << cmd_string

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

no need for cmd_name here.

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

it's not clear to me that the process taking longer than you expect it to should
be treated as an error.  certainly in the case of UninstallGoogleUpdate it's not
an error.  since you don't terminate the process, it may complete 1ns after your
timeout.  what then?

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

Done.

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

As discussed, will wait indefinitely.  However, this involves different flow, so
I undid the refactoring, and have semi-duplicated code.

+               << ") is taking more than " << kGoogleUpdateTimeoutMs
+               << " milliseconds to complete.";
+  }
+  return success;
+}
+
+}  // namespace
+
+bool EnsureUserLevelGoogleUpdateInstalled() {
+  LOG(INFO) << "Ensuring Google Update is installed at user level.";
+
+  if (IsGoogleUpdateInstalled(false)) {
+    LOG(INFO) << "Google Update is already installed.";

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

i don't think this is needed.  the log files get pretty big, so i think it's
good to take care not to put in information that's easily derived.  in this
case, the next log message after "Ensuring..." will make it clear whether it was
already installed or not.

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

Done.

+    return true;
+  } else {
+    string16 cmd_name(L"Google Update installer");

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

There's no need to pass around cmd_name. For the purposes of logs, the actual
command-line is sufficient.

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

Done.

+    string16 install_cmd(
+        GetUserLevelGoogleUpdateInstallCommandLine().GetCommandLineString());
+    base::win::ScopedHandle process;

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

i think this should check that the command string isn't empty before calling
Execute....  Execute itself doesn't need anything special to detect an empty
string since it'll eventually fail with an error.

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

Added check for empty string upstream.

+    return ExecuteGoogleUpdateCommand(install_cmd, cmd_name, &process) &&

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

please combine these two into one function that does the launch and wait.  i
don't see any reason to make callers have three lines of code instead of one.

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

Refactored another way!

+        WaitForGoogleUpdateCommand(install_cmd, cmd_name, process);
+  }
+}
+
+bool UninstallGoogleUpdate(bool system_install) {
+  string16 cmd_name(L"Google Update uninstaller");
+  string16 uninstall_cmd(
+      GoogleUpdateSettings::GetUninstallCommandLine(system_install));
+  if (!uninstall_cmd.empty()) {
+    base::win::ScopedHandle process;
+    return ExecuteGoogleUpdateCommand(uninstall_cmd, cmd_name, &process) &&
+        WaitForGoogleUpdateCommand(uninstall_cmd, cmd_name, process);
+  }
+  return true;
+}
+
+}  // namespace google_update