Switch on-demand update checks to the less-old GoogleUpdate3 API.

chrome/browser/google/google_update_win.cc

Index: chrome/browser/google/google_update_win.cc
diff --git a/chrome/browser/google/google_update_win.cc b/chrome/browser/google/google_update_win.cc
index 1e0d1b9c18b4b0b7ff7fbe10602f972a4754041a..fdd9c6b052dee4d3bbe319cb54cc9978a5bbc2c9 100644
--- a/chrome/browser/google/google_update_win.cc
+++ b/chrome/browser/google/google_update_win.cc
@@ -14,23 +14,30 @@
 #include "base/metrics/histogram.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/path_service.h"
+#include "base/sequenced_task_runner_helpers.h"
+#include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/task_runner.h"
 #include "base/thread_task_runner_handle.h"
+#include "base/time/time.h"
+#include "base/win/scoped_bstr.h"
 #include "base/win/windows_version.h"
 #include "chrome/grit/generated_resources.h"
 #include "chrome/installer/util/browser_distribution.h"
-#include "chrome/installer/util/google_update_settings.h"
 #include "chrome/installer/util/helper.h"
 #include "chrome/installer/util/install_util.h"
-#include "content/public/browser/browser_thread.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/win/atl_module.h"
 
 namespace {
 
-OnDemandAppsClassFactory* g_google_update_factory = nullptr;
+GoogleUpdate3ClassFactory* g_google_update_factory = nullptr;
+
+// Constants from Google Update.
+const HRESULT GOOPDATE_E_APP_UPDATE_DISABLED_BY_POLICY = 0x80040813;
+const HRESULT GOOPDATEINSTALL_E_INSTALLER_FAILED = 0x80040902;
 
 // Check if the currently running instance can be updated by Google Update.
 // Returns GOOGLE_UPDATE_NO_ERROR only if the instance running is a Google
@@ -52,18 +59,6 @@ GoogleUpdateErrorCode CanUpdateCurrentChrome(
     return CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY;
   }
 
-  base::string16 app_guid = installer::GetAppGuidForUpdates(system_level);
-  DCHECK(!app_guid.empty());
-
-  GoogleUpdateSettings::UpdatePolicy update_policy =
-      GoogleUpdateSettings::GetAppUpdatePolicy(app_guid, NULL);
-
-  if (update_policy == GoogleUpdateSettings::UPDATES_DISABLED)
-    return GOOGLE_UPDATE_DISABLED_BY_POLICY;
-
-  if (update_policy == GoogleUpdateSettings::AUTO_UPDATES_ONLY)
-    return GOOGLE_UPDATE_DISABLED_BY_POLICY_AUTO_ONLY;
-
   return GOOGLE_UPDATE_NO_ERROR;
 #endif
 }
@@ -107,182 +102,31 @@ HRESULT CoCreateInstanceAsAdmin(REFCLSID class_id,
                           interface_ptr);
 }
 
-HRESULT CreateOnDemandAppsClass(
+HRESULT CreateGoogleUpdate3Class(
     bool system_level,
     bool install_if_newer,
     gfx::AcceleratedWidget elevation_window,
-    base::win::ScopedComPtr<IGoogleUpdate>* on_demand) {
+    base::win::ScopedComPtr<IGoogleUpdate3>* google_update) {
   if (g_google_update_factory)
-    return g_google_update_factory->Run(on_demand);
+    return g_google_update_factory->Run(google_update);
 
   // For a user-level install, update checks and updates can both be done by a
-  // normal user with the UserAppsClass.
+  // normal user with the UserClass.
   if (!system_level)
-    return on_demand->CreateInstance(CLSID_OnDemandUserAppsClass);
+    return google_update->CreateInstance(CLSID_GoogleUpdate3UserClass);
 
   // For a system-level install, update checks can be done by a normal user with
-  // the MachineAppsClass.
+  // the ServiceClass.
   if (!install_if_newer)
-    return on_demand->CreateInstance(CLSID_OnDemandMachineAppsClass);
+    return google_update->CreateInstance(CLSID_GoogleUpdate3ServiceClass);
 
   // For a system-level install, an update requires Admin privileges for writing
-  // to %ProgramFiles%. Elevate while instantiating the MachineAppsClass.
-  return CoCreateInstanceAsAdmin(CLSID_OnDemandMachineAppsClass,
-                                 IID_IGoogleUpdate, elevation_window,
-                                 on_demand->ReceiveVoid());
-}
-
-
-// GoogleUpdateJobObserver -----------------------------------------------------
-
-// The GoogleUpdateJobObserver COM class is responsible for receiving status
-// reports from google Update. It keeps track of the progress as Google Update
-// notifies this observer and runs a completion callback once Google Update
-// reports that it is done.
-class GoogleUpdateJobObserver : public CComObjectRootEx<CComSingleThreadModel>,
-                                public IJobObserver {
- public:
-  BEGIN_COM_MAP(GoogleUpdateJobObserver)
-    COM_INTERFACE_ENTRY(IJobObserver)
-  END_COM_MAP()
-
-  GoogleUpdateJobObserver();
-  virtual ~GoogleUpdateJobObserver();
-
-  // Sets the callback to be invoked when Google Update reports that the job is
-  // done.
-  void set_on_complete_callback(const base::Closure& on_complete_callback) {
-    on_complete_callback_ = on_complete_callback;
-  }
-
-  // Returns the results of the update operation.
-  GoogleUpdateUpgradeResult result() const {
-    // Intermediary steps should never be reported to the client.
-    DCHECK_NE(UPGRADE_STARTED, result_);
-    DCHECK_NE(UPGRADE_CHECK_STARTED, result_);
-    return result_;
-  }
-
-  // Returns which version Google Update found on the server (if a more
-  // recent version was found). Otherwise, this will be blank.
-  base::string16 new_version() const { return new_version_; }
-
-  // Returns the Google Update supplied error string that describes the error
-  // that occurred during the update check/upgrade.
-  base::string16 error_message() const { return error_message_; }
-
- private:
-  // IJobObserver:
-  STDMETHOD(OnShow)() override;
-  STDMETHOD(OnCheckingForUpdate)() override;
-  STDMETHOD(OnUpdateAvailable)(const TCHAR* version_string) override;
-  STDMETHOD(OnWaitingToDownload)() override;
-  STDMETHOD(OnDownloading)(int time_remaining_ms, int pos) override;
-  STDMETHOD(OnWaitingToInstall)() override;
-  STDMETHOD(OnInstalling)() override;
-  STDMETHOD(OnPause)() override;
-  STDMETHOD(OnComplete)(LegacyCompletionCodes code, const TCHAR* text) override;
-  STDMETHOD(SetEventSink)(IProgressWndEvents* event_sink) override;
-
-  // The task runner associated with the thread in which the job runs.
-  scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
-
-  // A callback to be run to complete processing;
-  base::Closure on_complete_callback_;
-
-  // The status/result of the Google Update operation.
-  GoogleUpdateUpgradeResult result_;
-
-  // The version string Google Update found.
-  base::string16 new_version_;
-
-  // An error message, if any.
-  base::string16 error_message_;
-
-  // Allows us control the upgrade process to a small degree. After OnComplete
-  // has been called, this object can not be used.
-  base::win::ScopedComPtr<IProgressWndEvents> event_sink_;
-
-  DISALLOW_COPY_AND_ASSIGN(GoogleUpdateJobObserver);
-};
-
-GoogleUpdateJobObserver::GoogleUpdateJobObserver()
-    : task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      result_(UPGRADE_ERROR) {
-}
-
-GoogleUpdateJobObserver::~GoogleUpdateJobObserver() {
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnShow() {
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnCheckingForUpdate() {
-  result_ = UPGRADE_CHECK_STARTED;
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnUpdateAvailable(
-    const TCHAR* version_string) {
-  result_ = UPGRADE_IS_AVAILABLE;
-  new_version_ = version_string;
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnWaitingToDownload() {
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnDownloading(int time_remaining_ms,
-                                                    int pos) {
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnWaitingToInstall() {
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnInstalling() {
-  result_ = UPGRADE_STARTED;
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnPause() {
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::OnComplete(LegacyCompletionCodes code,
-                                                 const TCHAR* text) {
-  if (code == COMPLETION_CODE_ERROR) {
-    error_message_ = text;
-    result_ = UPGRADE_ERROR;
-  } else {
-    // Everything that isn't an error is some form of success. Chrome doesn't
-    // support any of the fancy codes (e.g., COMPLETION_CODE_REBOOT), but they
-    // shouldn't be generated anyway.
-    LOG_IF(DFATAL, (code != COMPLETION_CODE_SUCCESS &&
-                    code != COMPLETION_CODE_SUCCESS_CLOSE_UI))
-        << "Unexpected LegacyCompletionCode from IGoogleUpdate: " << code;
-    if (result_ == UPGRADE_STARTED)
-      result_ = UPGRADE_SUCCESSFUL;
-    else if (result_ == UPGRADE_CHECK_STARTED)
-      result_ = UPGRADE_ALREADY_UP_TO_DATE;
-  }
-
-  event_sink_ = NULL;
-
-  task_runner_->PostTask(FROM_HERE, on_complete_callback_);
-  return S_OK;
-}
-
-STDMETHODIMP GoogleUpdateJobObserver::SetEventSink(
-    IProgressWndEvents* event_sink) {
-  event_sink_ = event_sink;
-  return S_OK;
+  // to %ProgramFiles%. Elevate while instantiating the ServiceClass.
+  return CoCreateInstanceAsAdmin(CLSID_GoogleUpdate3ServiceClass,
+                                 IID_IGoogleUpdate3, elevation_window,
+                                 google_update->ReceiveVoid());
 }
 
-
 // UpdateCheckDriver -----------------------------------------------------------
 
 // A driver that is created and destroyed on the caller's thread and drives
@@ -293,6 +137,7 @@ class UpdateCheckDriver {
   // thread upon completion. |task_runner| must run a TYPE_UI message loop if
   // the default IGoogleUpdate on-demand COM class is used.
   static void RunUpdateCheck(const scoped_refptr<base::TaskRunner>& task_runner,
+                             const std::string& locale,
                              bool install_if_newer,
                              gfx::AcceleratedWidget elevation_window,
                              const UpdateCheckCallback& callback);
@@ -300,54 +145,133 @@ class UpdateCheckDriver {
  private:
   friend class base::DeleteHelper<UpdateCheckDriver>;
 
-  explicit UpdateCheckDriver(const UpdateCheckCallback& callback);
+  UpdateCheckDriver(const scoped_refptr<base::TaskRunner>& task_runner,
+                    const std::string& locale,
+                    bool install_if_newer,
+                    gfx::AcceleratedWidget elevation_window,
+                    const UpdateCheckCallback& callback);
 
   // Runs the caller's update check callback with the results of the operation.
   ~UpdateCheckDriver();
 
   // Starts an update check.
-  void BeginUpdateCheck(bool install_if_newer,
-                        gfx::AcceleratedWidget elevation_window);
+  void BeginUpdateCheck();
 
   // Helper function for starting an update check. Returns true if the check was
-  // properly started, in which case CompleteUpdateCheck will be invoked upon
-  // completion to return results to the caller on its own thread.
-  bool BeginUpdateCheckInternal(bool install_if_newer,
-                                gfx::AcceleratedWidget elevation_window);
-
-  // Invoked when results are in from Google Update.
-  void CompleteUpdateCheck();
+  // properly started.
+  bool BeginUpdateCheckInternal();
 
   // Prepares |results| to return the upgrade error indicated by |error_code|

[Peter Kasting, 2015/05/07 22:37:29]

What is |results|?  Does this mean |result_|?

[grt (UTC plus 2), 2015/05/08 18:51:50]

Comment revised.

   // and |hr|. The string " -- system level" is included in the generated error
-  // message when |system_level| is true.
+  // message when appropriate.
   void OnUpgradeError(GoogleUpdateErrorCode error_code,
                       HRESULT hr,

[Peter Kasting, 2015/05/07 22:37:28]

Nit: Some functions use |hr| and some |hresult| -- be consistent

[grt (UTC plus 2), 2015/05/08 18:51:50]

Done.

-                      bool system_level);
+                      int installer_exit_code,
+                      const base::string16& error_string);
+
+  // Returns true if |current_state| and |state_value| can be obtained from the
+  // ongoing update check. Otherwise, populates |hresult| with the reason they
+  // could not be obtained.

[Peter Kasting, 2015/05/07 22:37:29]

Is it possible to simply return the HRESULT, using SUCCESS where this would be
"true" and any other value otherwise?

[grt (UTC plus 2), 2015/05/08 18:51:49]

Yes, although I find that the logic in PollGoogleUpdate is a bit more clear with
these functions returning bools.

+  bool GetCurrentState(base::win::ScopedComPtr<ICurrentState>* current_state,
+                       CurrentState* state_value,
+                       HRESULT* hresult);
+
+  // Returns true if |current_state| and |state_value| constitute an error state
+  // for the ongoing update check, in which case |error_code| is populated with
+  // one of GOOGLE_UPDATE_ERROR_UPDATING, GOOGLE_UPDATE_DISABLED_BY_POLICY, or
+  // GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR. |hresult| is populated with
+  // the most relevant HRESULT (which may be a value from Google Update; see
+  // https://code.google.com/p/omaha/source/browse/trunk/base/error.h). In case
+  // Chrome's installer failed during execution, |installer_exit_code| may be
+  // populated with its process exit code (see enum installer::InstallStatus in
+  // chrome/installer/util/util_constants.h); otherwise, it will be -1.
+  // |error_string| will be populated with a completion message if one is
+  // provided by Google Update.
+  bool IsErrorState(const base::win::ScopedComPtr<ICurrentState>& current_state,
+                    CurrentState state_value,
+                    GoogleUpdateErrorCode* error_code,
+                    HRESULT* hresult,
+                    int* installer_exit_code,
+                    base::string16* error_string);
+
+  // Returns true if |current_state| and |state_value| constitute a final state
+  // for the ongoing update check, in which case |upgrade_result| is populated
+  // with one of UPGRADE_ALREADY_UP_TO_DATE or UPGRADE_IS_AVAILABLE (in case a
+  // pure check is being performed rather than an update) or UPGRADE_SUCCESSFUL
+  // (in case an update is being performed). For the UPGRADE_IS_AVAILABLE case,
+  // |new_version| will be populated with the available version, if provided by
+  // Google Update.
+  bool IsFinalState(const base::win::ScopedComPtr<ICurrentState>& current_state,
+                    CurrentState state_value,
+                    GoogleUpdateUpgradeResult* upgrade_result,
+                    base::string16* new_version);
+
+  // Returns true if |current_state| and |state_value| constitute an
+  // intermediate state for the ongoing update check, in which case
+  // |upgrade_result| is populated with UPGRADE_STARTED (in case an update is
+  // being performed) or UPGRADE_CHECK_STARTED (in case a pure check is being
+  // performed rather than an update). |new_version| will be populated with the
+  // version to be installed if it is provided by Google Update for the current
+  // state. |progress| will be populated with a number between 0.0 and 1.0
+  // according to how far Google Update has progressed in the download and
+  // install process.
+  bool IsIntermediateState(

[Peter Kasting, 2015/05/07 22:37:28]

Nit: Somehow conceptually it seems like this should go above the IsErrorState()
function, or maybe reverse the order of the three of these?  ("is intermediate,
final, or error state")

[grt (UTC plus 2), 2015/05/08 18:51:49]

I have them in this order to match the order in which they're called.

+      const base::win::ScopedComPtr<ICurrentState>& current_state,
+      CurrentState state_value,
+      GoogleUpdateUpgradeResult* upgrade_result,
+      base::string16* new_version,
+      double* progress);
+
+  // Pools Google Update to determine the state of the ongoing check or

[Peter Kasting, 2015/05/07 22:37:28]

Nit: Polls

[grt (UTC plus 2), 2015/05/08 18:51:49]

Doh!

+  // update. If the process has reached a terminal state, this instance will be
+  // deleted and the caller will be notified of the final status. Otherwise, the
+  // caller will be notified of the intermediate state (iff it differs from a
+  // previuos notification) and another future poll will be scheduled.

[Peter Kasting, 2015/05/07 22:37:29]

Nit: previous

[grt (UTC plus 2), 2015/05/08 18:51:49]

Done.

+  void PollGoogleUpdate();
+
+  // The task runner on which the update checks runs.
+  scoped_refptr<base::TaskRunner> task_runner_;
 
   // The caller's task runner, on which |result_callback_| will be run.
   scoped_refptr<base::SingleThreadTaskRunner> result_runner_;
 
+  // The UI locale.
+  std::string locale_;
+
+  // False to only check for an update; true to also install one if available.
+  bool install_if_newer_;

[Peter Kasting, 2015/05/07 22:37:28]

Nit: install_update_if_possible_ or install_available_updates_ might be clearer
names ("newer" is a bit ambiguous).  Same comment applies elsewhere

[grt (UTC plus 2), 2015/05/08 18:51:49]

Done.

+
+  // A parent window in case any UX is required (e.g., an elevation prompt).
+  gfx::AcceleratedWidget elevation_window_;
+
   // The caller's callback to be run when the update check is compelte.
   UpdateCheckCallback result_callback_;
 
-  // The results of the update check.
+  // True if operating at system-level.

[Peter Kasting, 2015/05/07 22:37:28]

Nit: "...instead of user-level" (I assume)

[grt (UTC plus 2), 2015/05/08 18:51:49]

Done.

+  bool system_level_;
+
+  // The on-demand updater that is doing the work.
+  base::win::ScopedComPtr<IGoogleUpdate3> google_update_;
+
+  // An app bundle containing the application being updated.
+  base::win::ScopedComPtr<IAppBundle> app_bundle_;
+
+  // The application being updated (Chrome, Chrome Binaries, or Chrome SxS).
+  base::win::ScopedComPtr<IApp> app_;
+
+  // The intermediate status of an ongoing operation that was most recently
+  // reported to the caller.
+  GoogleUpdateUpgradeResult last_result_;
+  double last_progress_;
+
+  // The results of the update check to be logged via UMA and/or reported to the
+  // caller.
   GoogleUpdateUpgradeResult result_;
   GoogleUpdateErrorCode error_code_;
   base::string16 error_message_;
-  base::string16 version_;
+  base::string16 new_version_;
   HRESULT hresult_;
-
-  // A direct pointer to the job observer by which the driver is notified of
-  // interesting events from Google Update.
-  CComObject<GoogleUpdateJobObserver>* job_observer_;
-
-  // A scoped pointer to |job_observer_| that holds a reference to it, keeping
-  // it alive.
-  base::win::ScopedComPtr<IJobObserver> job_holder_;
-
-  // The on-demand updater that is doing the work.
-  base::win::ScopedComPtr<IGoogleUpdate> on_demand_;
+  int installer_exit_code_;
 
   DISALLOW_COPY_AND_ASSIGN(UpdateCheckDriver);
 };
@@ -355,26 +279,39 @@ class UpdateCheckDriver {
 // static
 void UpdateCheckDriver::RunUpdateCheck(
     const scoped_refptr<base::TaskRunner>& task_runner,
+    const std::string& locale,
     bool install_if_newer,
     gfx::AcceleratedWidget elevation_window,
     const UpdateCheckCallback& callback) {
   // The driver is owned by itself, and will self-destruct when its work is
   // done.
-  UpdateCheckDriver* driver = new UpdateCheckDriver(callback);
-  task_runner->PostTask(
-      FROM_HERE,
-      base::Bind(&UpdateCheckDriver::BeginUpdateCheck, base::Unretained(driver),
-                 install_if_newer, elevation_window));
+  UpdateCheckDriver* driver = new UpdateCheckDriver(
+      task_runner, locale, install_if_newer, elevation_window, callback);
+  task_runner->PostTask(FROM_HERE,
+                        base::Bind(&UpdateCheckDriver::BeginUpdateCheck,
+                                   base::Unretained(driver)));
 }
 
 // Runs on the caller's thread.
-UpdateCheckDriver::UpdateCheckDriver(const UpdateCheckCallback& callback)
-    : result_runner_(base::ThreadTaskRunnerHandle::Get()),
+UpdateCheckDriver::UpdateCheckDriver(
+    const scoped_refptr<base::TaskRunner>& task_runner,
+    const std::string& locale,
+    bool install_if_newer,
+    gfx::AcceleratedWidget elevation_window,
+    const UpdateCheckCallback& callback)
+    : task_runner_(task_runner),
+      result_runner_(base::ThreadTaskRunnerHandle::Get()),
+      locale_(locale),
+      install_if_newer_(install_if_newer),
+      elevation_window_(elevation_window),
       result_callback_(callback),
+      system_level_(false),
+      last_result_(UPGRADE_ERROR),
+      last_progress_(0.0),
       result_(UPGRADE_ERROR),
       error_code_(GOOGLE_UPDATE_NO_ERROR),
       hresult_(S_OK),
-      job_observer_(nullptr) {
+      installer_exit_code_(-1) {
 }
 
 UpdateCheckDriver::~UpdateCheckDriver() {
@@ -386,36 +323,33 @@ UpdateCheckDriver::~UpdateCheckDriver() {
   if (result_ == UPGRADE_ERROR) {
     UMA_HISTOGRAM_ENUMERATION("GoogleUpdate.UpdateErrorCode", error_code_,
                               NUM_ERROR_CODES);
-    if (hresult_ != S_OK)
+    if (FAILED(hresult_))
       UMA_HISTOGRAM_SPARSE_SLOWLY("GoogleUpdate.ErrorHresult", hresult_);
+    if (installer_exit_code_ != -1) {
+      UMA_HISTOGRAM_SPARSE_SLOWLY("GoogleUpdate.InstallerExitCode",
+                                  installer_exit_code_);
+    }
   }
-  result_callback_.Run(result_, error_code_, error_message_, version_);
+  result_callback_.Run(result_, 0.0, error_code_, error_message_, new_version_);
 }
 
-void UpdateCheckDriver::BeginUpdateCheck(
-    bool install_if_newer,
-    gfx::AcceleratedWidget elevation_window) {
+void UpdateCheckDriver::BeginUpdateCheck() {
   // Return results immediately if the driver is not waiting for Google Update.
-  if (!BeginUpdateCheckInternal(install_if_newer, elevation_window))
+  if (!BeginUpdateCheckInternal())
     result_runner_->DeleteSoon(FROM_HERE, this);
 }
 
-bool UpdateCheckDriver::BeginUpdateCheckInternal(
-    bool install_if_newer,
-    gfx::AcceleratedWidget elevation_window) {
+bool UpdateCheckDriver::BeginUpdateCheckInternal() {
   base::FilePath chrome_exe;
   if (!PathService::Get(base::DIR_EXE, &chrome_exe))
     NOTREACHED();
 
-  const bool system_level = !InstallUtil::IsPerUserInstall(chrome_exe);
+  system_level_ = !InstallUtil::IsPerUserInstall(chrome_exe);

[Peter Kasting, 2015/05/07 22:37:29]

Nit: Maybe we should make the bool member be |per_user_install_| to match this
API (seems like a clearer name anyway)

[grt (UTC plus 2), 2015/05/08 18:51:50]

I agree that it seems backward, but system_level is more prevalent in the
codebase (especially in chrome/installer/* where it's most relevant).

[Peter Kasting, 2015/05/09 02:19:01]

OK; I would still suggest "system_level_install_" then as that seems a  bit
clearer.

[grt (UTC plus 2), 2015/05/12 20:21:51]

Done.

 
-  // The failures handled here are:
-  // CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY,
-  // GOOGLE_UPDATE_DISABLED_BY_POLICY, and
-  // GOOGLE_UPDATE_DISABLED_BY_POLICY_AUTO_ONLY.
-  error_code_ = CanUpdateCurrentChrome(chrome_exe, system_level);
+  // CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY is handled here.

[Peter Kasting, 2015/05/07 22:37:28]

Nit: This maybe should go inside the conditional

[grt (UTC plus 2), 2015/05/08 18:51:50]

It turns out that this comment was a lie. I've rearranged things a little so
that the error handling is consistent throughout this function.

+  error_code_ = CanUpdateCurrentChrome(chrome_exe, system_level_);
   if (error_code_ != GOOGLE_UPDATE_NO_ERROR) {
-    // These failures are handled in the UX with custom messages.
+    // This failure is handled in the UX with a custom message.
     result_ = UPGRADE_ERROR;
     return false;
   }
@@ -423,73 +357,335 @@ bool UpdateCheckDriver::BeginUpdateCheckInternal(
   // Make sure ATL is initialized in this module.
   ui::win::CreateATLModuleIfNeeded();
 
-  HRESULT hr =
-      CComObject<GoogleUpdateJobObserver>::CreateInstance(&job_observer_);
-  if (hr != S_OK) {
-    // Most of the error messages come straight from Google Update. This one is
-    // deemed worthy enough to also warrant its own error.
-    OnUpgradeError(GOOGLE_UPDATE_JOB_SERVER_CREATION_FAILED, hr, false);
+  // A failure to create the main update class is reported with this code:
+  GoogleUpdateErrorCode error_code = GOOGLE_UPDATE_ONDEMAND_CLASS_NOT_FOUND;
+  HRESULT hr = S_OK;
+  do {

[Peter Kasting, 2015/05/07 22:37:28]

A do { } while(false) construction here just so you can break; to the bottom of
it seems like a good indication that this should be a helper function that
return;s in these places.

(I would probably have the helper return the relevant HRESULT.)

[grt (UTC plus 2), 2015/05/08 18:51:49]

Done.

+    hr = CreateGoogleUpdate3Class(system_level_, install_if_newer_,
+                                  elevation_window_, &google_update_);
+    if (FAILED(hr)) {
+      DLOG(ERROR) << "failed to create GoogleUpdate3 class " << std::hex << hr;

[Peter Kasting, 2015/05/07 22:37:28]

Logging is uncommon in Chromium code and the old code didn't do any.  Normally
we can't collect the logs anyway and they just bloat the binary with the
relevant strings.  Avoid logging unless you need these for debugging (and plan
to remove them soon) or have a plan to collect the log output.

[grt (UTC plus 2), 2015/05/08 18:51:50]

All of these are DLOG, so there's no bloat. That said, I agree that there's not
much value to having them since anyone doing a debug build to look into a
problem here can use a debugger or add their own temporary logging. Removed.

+      break;
+    }
+
+    // The class was created, so all subsequent errors are reported as:
+    error_code = GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR;
+    base::string16 app_guid = installer::GetAppGuidForUpdates(system_level_);
+    DCHECK(!app_guid.empty());
+
+    base::win::ScopedComPtr<IDispatch> dispatch;
+    hr = google_update_->createAppBundle(dispatch.Receive());
+    if (FAILED(hr))
+      break;
+    hr = dispatch.QueryInterface(app_bundle_.Receive());
+    if (FAILED(hr))
+      break;
+    dispatch.Release();

[Peter Kasting, 2015/05/07 22:37:28]

Nit: I tend to think it's clearer to use something like:

  {
    base::win::ScopedComPtr<IDispatch> dispatch;
    ...
  }

...to indicate that the scope of the object is limited to here, and anything
below is unrelated.

[grt (UTC plus 2), 2015/05/08 18:51:49]

Done.

+
+    if (!locale_.empty()) {
+      hr = app_bundle_->put_displayLanguage(
+          base::win::ScopedBstr(base::UTF8ToUTF16(locale_).c_str()));
+      DLOG_IF(ERROR, FAILED(hr)) << std::hex << hr;
+    }

[Peter Kasting, 2015/05/07 22:37:29]

Nit: Maybe blank line below this?  Not sure, but it seems like there should at
least be some blanks inserted in the rest of this block as everything is just
strung together

[grt (UTC plus 2), 2015/05/08 18:51:50]

Done.

+    // This doesn't work. Perhaps it isn't needed since elevation is done by

[Peter Kasting, 2015/05/07 22:37:29]

What does "doesn't work" mean?

Also, how is this distinct from when we've already passed this HWND to
CoCreateInstanceAsAdmin()?  Is that not sufficient?

Normally we don't leave commented-out code in unless we're going to re-enable it
later, so preferable would be to say something in English like "There's no need
to call IAppBundle::puParentHWND() here because ...".

[grt (UTC plus 2), 2015/05/08 18:51:49]

Causes Google Update to crash. :-(
I believe so. I chatted with Ganesh and verified that there's unlikely to be a
case where Google Update would want to pop any kind of UX.
I'll just remove the whole thing. I don't think the comment is really needed.
There are many other functions on these COM interfaces that aren't being used
here.

+    // Chrome.
+    // hr = app_bundle_->put_parentHWND(
+    //     reinterpret_cast<ULONG_PTR>(elevation_window_));
+    // DLOG_IF(ERROR, FAILED(hr)) << std::hex << hr;
+    hr = app_bundle_->initialize();
+    if (FAILED(hr)) {
+      DLOG(ERROR) << "initialize failed " << std::hex << hr;
+      break;
+    }
+    hr = app_bundle_->createInstalledApp(
+        base::win::ScopedBstr(app_guid.c_str()), dispatch.Receive());
+    if (FAILED(hr)) {
+      DLOG(ERROR) << "createInstalledApp failed " << std::hex << hr;
+      break;
+    }
+    hr = dispatch.QueryInterface(app_.Receive());
+    if (FAILED(hr))
+      break;
+    hr = app_bundle_->checkForUpdate();
+    DLOG_IF(ERROR, FAILED(hr)) << "checkForUpdate failed " << std::hex << hr;
+  } while (false);
+
+  if (FAILED(hr)) {
+    OnUpgradeError(error_code, hr, -1, base::string16());
     return false;
   }
-  // Hold a reference on the observer for the lifetime of the driver.
-  job_holder_ = job_observer_;
 
-  job_observer_->set_on_complete_callback(base::Bind(
-      &UpdateCheckDriver::CompleteUpdateCheck, base::Unretained(this)));
+  task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&UpdateCheckDriver::PollGoogleUpdate, base::Unretained(this)));
+
+  return true;
+}
 
-  hr = CreateOnDemandAppsClass(system_level, install_if_newer, elevation_window,
-                               &on_demand_);
-  if (hr != S_OK) {
-    OnUpgradeError(GOOGLE_UPDATE_ONDEMAND_CLASS_NOT_FOUND, hr, system_level);
+bool UpdateCheckDriver::GetCurrentState(
+    base::win::ScopedComPtr<ICurrentState>* current_state,
+    CurrentState* state_value,
+    HRESULT* hresult) {
+  base::win::ScopedComPtr<IDispatch> dispatch;
+  *hresult = app_->get_currentState(dispatch.Receive());
+  if (FAILED(*hresult)) {
+    DLOG(ERROR) << "failed getting currentState " << std::hex << *hresult;
+    return false;
+  }
+  *hresult = dispatch.QueryInterface(current_state->Receive());
+  if (FAILED(*hresult)) {
+    DLOG(ERROR) << "failed getting ICurrentState " << std::hex << *hresult;
+    return false;
+  }
+  dispatch.Release();

[Peter Kasting, 2015/05/07 22:37:28]

Why is this call needed?

[grt (UTC plus 2), 2015/05/08 18:51:49]

For consistency with the previous uses of IDispatch above. Removed in favor of
introducing a scope for the IDispatch as above.

[Peter Kasting, 2015/05/09 02:19:01]

I guess the question is more, either a Release() or a scope makes it seem to the
reader that it's important that this be destroyed by this point.  In the case
above, that's true, because we want to avoid overlapping the lifetime of two
temps.  In this case, though, it seems like this could just persist to the end
of the function.  So even an explicit scope seems kinda misleading.  I'd just
let this be function-scoped.

[grt (UTC plus 2), 2015/05/12 20:21:51]

Done.

+  LONG value = 0;
+  *hresult = (*current_state)->get_stateValue(&value);
+  if (FAILED(*hresult)) {
+    DLOG(ERROR) << "failed getting state value " << std::hex << *hresult;
     return false;
   }
+  *state_value = static_cast<CurrentState>(value);
+  return true;
+}
 
-  base::string16 app_guid = installer::GetAppGuidForUpdates(system_level);
-  DCHECK(!app_guid.empty());
+bool UpdateCheckDriver::IsErrorState(
+    const base::win::ScopedComPtr<ICurrentState>& current_state,
+    CurrentState state_value,
+    GoogleUpdateErrorCode* error_code,
+    HRESULT* hresult,
+    int* installer_exit_code,
+    base::string16* error_string) {
+  if (state_value == STATE_ERROR) {
+    // In general, errors reported by Google Update fall under this category
+    // (see special case below).
+    *error_code = GOOGLE_UPDATE_ERROR_UPDATING;
+
+    // In general, the exit code of Chrome's installer is unknown (see special
+    // case below).
+    *installer_exit_code = -1;
+
+    // Report the error_code provided by Google Update if possible, or the
+    // reason it wasn't possible otherwise.
+    LONG long_value = 0;
+    *hresult = current_state->get_errorCode(&long_value);
+    if (SUCCEEDED(*hresult))
+      *hresult = long_value;
+
+    // Special cases:
+    // - Use a custom error code if Google Update repoted that the update was
+    //   disabled by a Group Policy setting.
+    // - Extract the exit code of Chrome's installer if Google Update repoted
+    //   that the update failed becuase of a failure in the installer.

[Peter Kasting, 2015/05/07 22:37:28]

Nit: because

[grt (UTC plus 2), 2015/05/08 18:51:49]

Done.

+    LONG code = 0;
+    if (*hresult == GOOPDATE_E_APP_UPDATE_DISABLED_BY_POLICY) {
+      *error_code = GOOGLE_UPDATE_DISABLED_BY_POLICY;
+    } else if (*hresult == GOOPDATEINSTALL_E_INSTALLER_FAILED &&
+               SUCCEEDED(current_state->get_installerResultCode(&code))) {
+      *installer_exit_code = code;
+    }
+
+    base::win::ScopedBstr message;
+    if (SUCCEEDED(current_state->get_completionMessage(message.Receive())))
+      error_string->assign(message, message.Length());
+
+    return true;
+  }
+  if (state_value == STATE_UPDATE_AVAILABLE && install_if_newer_) {
+    *hresult = app_bundle_->install();
+    if (FAILED(*hresult)) {
+      DLOG(ERROR) << "install() failed " << std::hex << *hresult;
+      // Report a failure to start the install as a general error while trying
+      // to interact with Google Update.
+      *error_code = GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR;
+      *installer_exit_code = -1;
+      return true;
+    }
+    // Return false for handling in IsIntermediateState.
+  }
+  return false;
+}
 
-  if (install_if_newer)
-    hr = on_demand_->Update(app_guid.c_str(), job_observer_);
-  else
-    hr = on_demand_->CheckForUpdate(app_guid.c_str(), job_observer_);
+bool UpdateCheckDriver::IsFinalState(
+    const base::win::ScopedComPtr<ICurrentState>& current_state,
+    CurrentState state_value,
+    GoogleUpdateUpgradeResult* upgrade_result,
+    base::string16* new_version) {
+  if (state_value == STATE_UPDATE_AVAILABLE && !install_if_newer_) {
+    base::win::ScopedBstr version;
+    *upgrade_result = UPGRADE_IS_AVAILABLE;
+    if (SUCCEEDED(current_state->get_availableVersion(version.Receive())))
+      new_version->assign(version, version.Length());
+    return true;
+  } else if (state_value == STATE_INSTALL_COMPLETE) {

[Peter Kasting, 2015/05/07 22:37:28]

Nit: No else after return (2 places)

[grt (UTC plus 2), 2015/05/08 18:51:50]

Done.

+    DCHECK(install_if_newer_);
+    *upgrade_result = UPGRADE_SUCCESSFUL;
+    return true;
+  } else if (state_value == STATE_NO_UPDATE) {
+    *upgrade_result = UPGRADE_ALREADY_UP_TO_DATE;
+    return true;
+  }
+  return false;
+}
 
-  if (hr != S_OK) {
-    OnUpgradeError(GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR, hr,
-                   system_level);
-    return false;
+bool UpdateCheckDriver::IsIntermediateState(
+    const base::win::ScopedComPtr<ICurrentState>& current_state,
+    CurrentState state_value,
+    GoogleUpdateUpgradeResult* upgrade_result,
+    base::string16* new_version,
+    double* progress) {
+  // ERROR will have been handled in IsErrorState. UPDATE_AVAILABLE, and
+  // NO_UPDATE will have been handled in IsFinalState if not doing an install,
+  // as will STATE_INSTALL_COMPLETE when doing an install. All other states
+  // following UPDATE_AVAILABLE will only happen when an install is to be done.
+  DCHECK(state_value < STATE_UPDATE_AVAILABLE || install_if_newer_);
+  *upgrade_result = install_if_newer_ ? UPGRADE_STARTED : UPGRADE_CHECK_STARTED;
+  *progress = 0.0;
+
+  switch (state_value) {
+    case STATE_INIT:
+    case STATE_WAITING_TO_CHECK_FOR_UPDATE:
+    case STATE_CHECKING_FOR_UPDATE:
+      break;
+
+    case STATE_UPDATE_AVAILABLE: {
+      base::win::ScopedBstr version;
+      if (SUCCEEDED(current_state->get_availableVersion(version.Receive())))
+        new_version->assign(version, version.Length());
+      break;
+    }
+
+    case STATE_WAITING_TO_DOWNLOAD:
+    case STATE_RETRYING_DOWNLOAD:
+      break;
+
+    case STATE_DOWNLOADING: {
+      ULONG bytes_downloaded = 0;
+      ULONG total_bytes = 0;
+      if (SUCCEEDED(current_state->get_bytesDownloaded(&bytes_downloaded)) &&
+          SUCCEEDED(current_state->get_totalBytesToDownload(&total_bytes)) &&
+          total_bytes) {
+        // Divide by two, as 0-50% is downloading.
+        *progress = (static_cast<double>(bytes_downloaded) /
+                     static_cast<double>(total_bytes)) /
+                    2.0;
+      }
+      break;
+    }
+
+    case STATE_DOWNLOAD_COMPLETE:
+    case STATE_EXTRACTING:
+    case STATE_APPLYING_DIFFERENTIAL_PATCH:
+    case STATE_READY_TO_INSTALL:
+    case STATE_WAITING_TO_INSTALL:
+      *progress = 0.5;
+      break;
+
+    case STATE_INSTALLING: {
+      *progress = 0.5;
+      LONG install_progress = 0;
+      if (SUCCEEDED(current_state->get_installProgress(&install_progress)) &&
+          install_progress >= 0 && install_progress <= 100) {

[Peter Kasting, 2015/05/07 22:37:28]

Seems like the effect of this is to clamp all out-of-range values to 0.  I
wonder if something like ClampToRange() (in cc/base/math_util.h) might be
better, or if we need to clamp this at all; is this ever going to return
out-of-range values?

[grt (UTC plus 2), 2015/05/08 18:51:49]

I can't say if a flaw in Google Update will ever cause it to return a value out
of range. It seems like reasonable defensive programming to check for an
expected value. This is meant to treat an out-of-range value the same way as a
failure to get the value, not to clamp it to the min or max, whichever is
appropriate.

[Peter Kasting, 2015/05/09 02:19:01]

OK, fair enough.

+        // 50-100% is installing.
+        *progress = (100.0 + install_progress) / 200.0;
+      }
+      break;
+    }
+
+    case STATE_INSTALL_COMPLETE:
+    case STATE_PAUSED:
+    case STATE_NO_UPDATE:
+    case STATE_ERROR:
+    default:
+      NOTREACHED();
+      UMA_HISTOGRAM_SPARSE_SLOWLY("GoogleUpdate.UnexpectedState", state_value);
+      return false;
   }
   return true;
 }
 
-void UpdateCheckDriver::CompleteUpdateCheck() {
-  result_ = job_observer_->result();
-  if (result_ == UPGRADE_ERROR) {
-    error_code_ = GOOGLE_UPDATE_ERROR_UPDATING;
-    error_message_ = job_observer_->error_message();
-  } else {
-    version_ = job_observer_->new_version();
+void UpdateCheckDriver::PollGoogleUpdate() {
+  base::win::ScopedComPtr<ICurrentState> state;
+  CurrentState state_value = STATE_INIT;
+  HRESULT hr = S_OK;
+

[Peter Kasting, 2015/05/07 22:37:28]

Nit: This blank line and the two below seem a bit random

[grt (UTC plus 2), 2015/05/08 18:51:49]

Removed.

+  GoogleUpdateErrorCode error_code = GOOGLE_UPDATE_NO_ERROR;
+  int installer_exit_code = -1;
+  base::string16 error_string;
+
+  GoogleUpdateUpgradeResult upgrade_result = UPGRADE_ERROR;
+  base::string16 new_version;
+
+  double progress = 0.0;
+
+  if (!GetCurrentState(&state, &state_value, &hr)) {
+    OnUpgradeError(GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR, hr, -1,
+                   base::string16());
+  } else if (IsErrorState(state, state_value, &error_code, &hr,
+                          &installer_exit_code, &error_string)) {
+    OnUpgradeError(error_code, hr, installer_exit_code, error_string);
+  } else if (IsFinalState(state, state_value, &upgrade_result, &new_version)) {
+    result_ = upgrade_result;
+    error_code_ = GOOGLE_UPDATE_NO_ERROR;
+    error_message_.clear();
+    if (!new_version.empty())
+      new_version_ = new_version;
+    hresult_ = S_OK;
+    installer_exit_code_ = -1;
+  } else if (IsIntermediateState(state, state_value, &upgrade_result,
+                                 &new_version, &progress)) {
+    if (!new_version.empty())
+      new_version_ = new_version;
+    // Give the caller this status update if it differs from the last one given
+    // and schedule the next check.
+    if (upgrade_result != last_result_ || progress != last_progress_) {
+      last_result_ = upgrade_result;
+      last_progress_ = progress;
+
+      result_runner_->PostTask(
+          FROM_HERE,
+          base::Bind(result_callback_, last_result_, last_progress_,
+                     GOOGLE_UPDATE_NO_ERROR, base::string16(), new_version_));
+    }
+
+    task_runner_->PostDelayedTask(
+        FROM_HERE, base::Bind(&UpdateCheckDriver::PollGoogleUpdate,
+                              base::Unretained(this)),
+        base::TimeDelta::FromMilliseconds(250L));

[Peter Kasting, 2015/05/07 22:37:28]

Nit: Explicit 'L' suffix not necessary

You may want to comment why we have a 250 ms delay (and not 0, or some other
value)?

[grt (UTC plus 2), 2015/05/08 18:51:49]

Done.

+    // Early return for this non-terminal state.
+    return;
   }
 
   // Release the reference on the COM objects before bouncing back to the
   // caller's thread.
-  on_demand_.Release();
-  job_holder_.Release();
-  job_observer_ = nullptr;
+  state.Release();
+  app_.Release();
+  app_bundle_.Release();
+  google_update_.Release();
 
   result_runner_->DeleteSoon(FROM_HERE, this);
 }
 
 void UpdateCheckDriver::OnUpgradeError(GoogleUpdateErrorCode error_code,
                                        HRESULT hr,
-                                       bool system_level) {
+                                       int installer_exit_code,
+                                       const base::string16& error_string) {
   result_ = UPGRADE_ERROR;
   error_code_ = error_code;
   hresult_ = hr;
+  installer_exit_code_ = installer_exit_code;
   base::string16 error_msg =
       base::StringPrintf(L"%d: 0x%x", error_code_, hresult_);
-  if (system_level)
+  if (installer_exit_code_ != -1)
+    error_msg += base::StringPrintf(L": %d", installer_exit_code_);
+  if (system_level_)
     error_msg += L" -- system level";
-  error_message_ = l10n_util::GetStringFUTF16(
-      IDS_ABOUT_BOX_ERROR_UPDATE_CHECK_FAILED, error_msg);
+  if (error_string.empty()) {
+    error_message_ = l10n_util::GetStringFUTF16(
+        IDS_ABOUT_BOX_ERROR_UPDATE_CHECK_FAILED, error_msg);
+  } else {
+    error_message_ = l10n_util::GetStringFUTF16(
+        IDS_ABOUT_BOX_GOOGLE_UPDATE_ERROR, error_string, error_msg);
+  }
 }
 
 }  // namespace
@@ -498,10 +694,11 @@ void UpdateCheckDriver::OnUpgradeError(GoogleUpdateErrorCode error_code,
 // Globals ---------------------------------------------------------------------
 
 void BeginUpdateCheck(const scoped_refptr<base::TaskRunner>& task_runner,
+                      const std::string& locale,
                       bool install_if_newer,
                       gfx::AcceleratedWidget elevation_window,
                       const UpdateCheckCallback& callback) {
-  UpdateCheckDriver::RunUpdateCheck(task_runner, install_if_newer,
+  UpdateCheckDriver::RunUpdateCheck(task_runner, locale, install_if_newer,
                                     elevation_window, callback);
 }
 
@@ -509,13 +706,13 @@ void BeginUpdateCheck(const scoped_refptr<base::TaskRunner>& task_runner,
 // Private API exposed for testing. --------------------------------------------
 
 void SetGoogleUpdateFactoryForTesting(
-    const OnDemandAppsClassFactory& google_update_factory) {
+    const GoogleUpdate3ClassFactory& google_update_factory) {
   if (g_google_update_factory) {
     delete g_google_update_factory;
     g_google_update_factory = nullptr;
   }
   if (!google_update_factory.is_null()) {
     g_google_update_factory =
-        new OnDemandAppsClassFactory(google_update_factory);
+        new GoogleUpdate3ClassFactory(google_update_factory);
   }
 }