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

chrome/browser/ui/webui/help/version_updater_win.cc

Index: chrome/browser/ui/webui/help/version_updater_win.cc
diff --git a/chrome/browser/ui/webui/help/version_updater_win.cc b/chrome/browser/ui/webui/help/version_updater_win.cc
index 0d9b5286675d75f8dfc92423467e1988d77aec95..eb6e1ce1eb8bf52f520ddb63dcd228b07ab6bed9 100644
--- a/chrome/browser/ui/webui/help/version_updater_win.cc
+++ b/chrome/browser/ui/webui/help/version_updater_win.cc
@@ -2,64 +2,62 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "base/memory/ref_counted.h"
-#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
-#include "base/version.h"
 #include "base/win/win_util.h"
 #include "base/win/windows_version.h"
+#include "chrome/browser/browser_process.h"
+#include "chrome/browser/first_run/upgrade_util_win.h"
 #include "chrome/browser/google/google_update_win.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
-#include "chrome/browser/ui/browser.h"
 #include "chrome/browser/ui/webui/help/version_updater.h"
-#include "chrome/common/chrome_version_info.h"
-#include "chrome/grit/chromium_strings.h"
 #include "chrome/grit/generated_resources.h"
-#include "chrome/installer/util/browser_distribution.h"
-#include "chrome/installer/util/install_util.h"
 #include "content/public/browser/browser_thread.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/gfx/native_widget_types.h"
-#include "ui/views/widget/widget.h"
-
-using content::BrowserThread;
 
 namespace {
 
+// An EnumThreadWndProc that returns the first visible window via |param|,
+// stopping the enumeration when one is found.
+BOOL CALLBACK WindowEnumeration(HWND window, LPARAM param) {
+  if (!IsWindowVisible(window))
+    return TRUE;
+  HWND* returned_window = reinterpret_cast<HWND*>(param);

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

Nit: I'd probably just inline this into the next line.

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

Done.

+  *returned_window = window;
+  return FALSE;
+}
+
+// Returns a visible Chrome Window, or null if none is found.
+gfx::AcceleratedWidget FindVisibleWindow() {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  HWND window = nullptr;
+  EnumThreadWindows(GetCurrentThreadId(), &WindowEnumeration,

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

It makes me uncomfortable to have Windows calls doing this.  Can't we use normal
Browser methods and the like to get the relevant current window for this browser
process?

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

Done. There's a slight change in behavior, but after a long discussion with
Siggi, I believe it's the right thing. I've documented the behavior in
VersionUpdater::Create.

+                    reinterpret_cast<LPARAM>(&window));
+  return window;
+}
+
 // Windows implementation of version update functionality, used by the WebUI
 // About/Help page.
-class VersionUpdaterWin : public VersionUpdater {
- private:
-  friend class VersionReader;
-  friend class VersionUpdater;
-
-  // Clients must use VersionUpdater::Create().
+class VersionUpdaterWin : public VersionUpdater, public UpdateCheckDelegate {
+ public:
   VersionUpdaterWin();
   ~VersionUpdaterWin() override;
 
-  // VersionUpdater implementation.
+  // VersionUpdater

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

Tiny nit: I'd put a colon after these base class names

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

Done.

   void CheckForUpdate(const StatusCallback& callback) override;
   void RelaunchBrowser() const override;
 
-  // chrome::UpdateCheckCallback.
-  void OnUpdateCheckResults(GoogleUpdateUpgradeResult result,
-                            GoogleUpdateErrorCode error_code,
-                            const base::string16& error_message,
-                            const base::string16& version);
-
-  // Update the UI to show the status of the upgrade.
-  void UpdateStatus(GoogleUpdateUpgradeResult result,
-                    GoogleUpdateErrorCode error_code,
-                    const base::string16& error_message);
-
-  // Got the intalled version so the handling of the UPGRADE_ALREADY_UP_TO_DATE
-  // result case can now be completeb on the UI thread.
-  void GotInstalledVersion(const Version& version);
-
-  // Returns a window that can be used for elevation.
-  gfx::AcceleratedWidget GetElevationParent();
+  // UpdateCheckDelegate
+  void OnUpdateCheckComplete(const base::string16& new_version) override;
+  void OnUpgradeProgress(int progress,
+                         const base::string16& new_version) override;
+  void OnUpgradeComplete(const base::string16& new_version) override;
+  void OnError(GoogleUpdateErrorCode error_code,
+               const base::string16& error_message,
+               const base::string16& new_version) override;
 
+ private:
   void BeginUpdateCheckOnFileThread(bool install_if_newer);

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

Nit: Probably want to change the name of |install_if_newer| to match what you
did in the other file

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

Done.

 
   // Callback used to communicate update status to the client.
@@ -71,49 +69,6 @@ class VersionUpdaterWin : public VersionUpdater {
   DISALLOW_COPY_AND_ASSIGN(VersionUpdaterWin);
 };
 
-// This class is used to read the version on the FILE thread and then call back
-// the version updater in the UI thread. Using a class helps better control
-// the lifespan of the Version independently of the lifespan of the version
-// updater, which may die while asynchonicity is happening, thus the usage of
-// the WeakPtr, which can only be used from the thread that created it.
-class VersionReader
-    : public base::RefCountedThreadSafe<VersionReader> {
- public:
-  explicit VersionReader(
-      const base::WeakPtr<VersionUpdaterWin>& version_updater)
-      : version_updater_(version_updater) {
-  }
-
-  void GetVersionFromFileThread() {
-    BrowserDistribution* dist = BrowserDistribution::GetDistribution();
-    InstallUtil::GetChromeVersion(dist, false, &installed_version_);
-    if (!installed_version_.IsValid()) {
-      // User-level Chrome is not installed, check system-level.
-      InstallUtil::GetChromeVersion(dist, true, &installed_version_);
-    }
-    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, base::Bind(
-          &VersionReader::SetVersionInUIThread, this));
-  }
-
-  void SetVersionInUIThread() {
-    if (version_updater_.get() != NULL)
-      version_updater_->GotInstalledVersion(installed_version_);
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<VersionReader>;
-
-  ~VersionReader() {}
-
-  // The version updater that must be called back when we are done.
-  // We use a weak pointer in case the updater gets destroyed while waiting.
-  base::WeakPtr<VersionUpdaterWin> version_updater_;
-
-  // This is the version that gets read in the FILE thread and set on the
-  // the updater in the UI thread.
-  Version installed_version_;
-};
-
 VersionUpdaterWin::VersionUpdaterWin()
     : weak_factory_(this) {
 }
@@ -132,10 +87,8 @@ void VersionUpdaterWin::CheckForUpdate(const StatusCallback& callback) {
   if (!(base::win::GetVersion() == base::win::VERSION_VISTA &&
         (base::win::OSInfo::GetInstance()->service_pack().major == 0) &&
         !base::win::UserAccountControlIsEnabled())) {
-    UpdateStatus(UPGRADE_CHECK_STARTED, GOOGLE_UPDATE_NO_ERROR,
-                 base::string16());
-    // Specify false to not upgrade yet.
-    BeginUpdateCheckOnFileThread(false);
+    callback_.Run(CHECKING, 0, base::string16());
+    BeginUpdateCheckOnFileThread(false /* !install_if_newer */);

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

Nit: I'm not in love with these comments when there's only one arg... I feel
like people can look at the function declaration if they need to know what this
is.

If you thought this was so unclear that it required a comment, then at that
point I'd switch from a bool to an enum so as to pass named args (but I don't
think that's needed here).

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

I've been asked by other reviewers to add these comments repeatedly, so I've
gotten in the habit of doing so by default. I don't think it's a great challenge
for the reader to look at the function declaration as you suggested, but I also
don't think that the comment is noise.

   }
 }
 
@@ -143,126 +96,64 @@ void VersionUpdaterWin::RelaunchBrowser() const {
   chrome::AttemptRestart();
 }
 
-void VersionUpdaterWin::OnUpdateCheckResults(
-    GoogleUpdateUpgradeResult result,
-    GoogleUpdateErrorCode error_code,
-    const base::string16& error_message,
-    const base::string16& version) {
-  UpdateStatus(result, error_code, error_message);
+void VersionUpdaterWin::OnUpdateCheckComplete(
+    const base::string16& new_version) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  Status status = CHECKING;
+  if (new_version.empty()) {
+    // Google Update says that no new version is available. Check to see if a
+    // restart is needed for it to take effect.

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

Nit: Maybe "Check if we need to restart to pick up a previously-applied update"?
 The "it" in this sentence has no real antecedent.

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

Done.

+    status = upgrade_util::IsRunningOldChrome() ? NEARLY_UPDATED : UPDATED;
+  } else {
+    // Notify the caller that the update is now beginning and initiate it.
+    status = UPDATING;
+    BeginUpdateCheckOnFileThread(true /* install_if_newer */);
+  }
+  callback_.Run(status, 0, base::string16());
 }
 
-void VersionUpdaterWin::UpdateStatus(GoogleUpdateUpgradeResult result,
-                                     GoogleUpdateErrorCode error_code,
-                                     const base::string16& error_message) {
-  // For Chromium builds it would show an error message.
-  // But it looks weird because in fact there is no error,
-  // just the update server is not available for non-official builds.
-#if defined(GOOGLE_CHROME_BUILD)
-  Status status = UPDATED;
-  base::string16 message;
-
-  switch (result) {
-    case UPGRADE_CHECK_STARTED: {
-      status = CHECKING;
-      break;
-    }
-    case UPGRADE_STARTED: {
-      status = UPDATING;
-      break;
-    }
-    case UPGRADE_IS_AVAILABLE: {
-      UpdateStatus(UPGRADE_STARTED, GOOGLE_UPDATE_NO_ERROR, base::string16());
-      // Specify true to upgrade now.
-      BeginUpdateCheckOnFileThread(true);
-      return;
-    }
-    case UPGRADE_ALREADY_UP_TO_DATE: {
-      // Google Update reported that Chrome is up-to-date.
-      // To confirm the updated version is running, the reading
-      // must be done on the file thread. The rest of this case
-      // will be handled within GotInstalledVersion.
-      BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
-            &VersionReader::GetVersionFromFileThread,
-            new VersionReader(weak_factory_.GetWeakPtr())));
-      return;
-    }
-    case UPGRADE_SUCCESSFUL: {
-      status = NEARLY_UPDATED;
-      break;
-    }
-    case UPGRADE_ERROR: {
-      status = FAILED;
-      if (error_code == GOOGLE_UPDATE_DISABLED_BY_POLICY) {
-        message =
-            l10n_util::GetStringUTF16(IDS_UPGRADE_DISABLED_BY_POLICY);
-      } else if (error_code == GOOGLE_UPDATE_DISABLED_BY_POLICY_AUTO_ONLY) {
-        message =
-            l10n_util::GetStringUTF16(IDS_UPGRADE_DISABLED_BY_POLICY_MANUAL);
-      } else {
-        message =
-            l10n_util::GetStringFUTF16Int(IDS_UPGRADE_ERROR, error_code);
-      }
-
-      if (!error_message.empty()) {
-        message +=
-            l10n_util::GetStringFUTF16(IDS_ABOUT_BOX_ERROR_DURING_UPDATE_CHECK,
-                                       error_message);
-      }
-      break;
-    }
-  }
+void VersionUpdaterWin::OnUpgradeProgress(int progress,
+                                          const base::string16& new_version) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  callback_.Run(UPDATING, progress, base::string16());
+}
 
-  // TODO(mad): Get proper progress value instead of passing 0.
-  // http://crbug.com/136117
-  callback_.Run(status, 0, message);
-#endif  //  defined(GOOGLE_CHROME_BUILD)
+void VersionUpdaterWin::OnUpgradeComplete(const base::string16& new_version) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  callback_.Run(NEARLY_UPDATED, 0, base::string16());
 }
 
-void VersionUpdaterWin::GotInstalledVersion(const Version& version) {
-  // This must be called on the UI thread so that callback_ can be called.
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+void VersionUpdaterWin::OnError(GoogleUpdateErrorCode error_code,
+                                const base::string16& error_message,
+                                const base::string16& new_version) {
+  // Chromium builds will unconditionally get a
+  // CANNOT_UPGRADE_CHROME_IN_THIS_DIRECTORY error since there is no supported
+  // integration with Google Update for Chromium. Ignore the error notification
+  // so that the spinner just spins rather than show an error message when there

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

Nit: showing

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

Done.

+  // is, in fact, no error.

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

Is there a way to not spin but instead show the "no update is available" UI
instead?

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

I make a chromium-branded build and found that I was wrong about the behavior.
I've updated the comment.

+#if defined(GOOGLE_CHROME_BUILD)
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  base::string16 message;
 
-  // Make sure that the latest version is running and if not,
-  // notify the user by setting the status to NEARLY_UPDATED.
-  //
-  // The extra version check is necessary on Windows because the application
-  // may be already up to date on disk though the running app is still
-  // out of date.
-  chrome::VersionInfo version_info;
-  Version running_version(version_info.Version());
-  callback_.Run((version.IsValid() && version.CompareTo(running_version) > 0)
-                    ? NEARLY_UPDATED
-                    : UPDATED,
-                0,
-                base::string16());
-}
+  // Google Update provides a nice message for the policy case. Use this

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

Nit: "usually provides"?  (If it always provided this, we wouldn't have to
handle the case where it didn't.)

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

Just being defensive here. I don't believe Google Update has a contract anywhere
stating that there definitely will be a good message, but there is for
up-to-date versions. I've clarified the comment.

+  // generic error for the policy case only if no message from Google Update
+  // is present.
+  if (error_code != GOOGLE_UPDATE_DISABLED_BY_POLICY || error_message.empty())
+    message = l10n_util::GetStringFUTF16Int(IDS_UPGRADE_ERROR, error_code);
 
-BOOL CALLBACK WindowEnumeration(HWND window, LPARAM param) {
-  if (IsWindowVisible(window)) {
-    HWND* returned_window = reinterpret_cast<HWND*>(param);
-    *returned_window = window;
-    return FALSE;
+  if (!error_message.empty()) {
+    message += l10n_util::GetStringFUTF16(
+        IDS_ABOUT_BOX_ERROR_DURING_UPDATE_CHECK, error_message);
   }
-  return TRUE;
-}
-
-gfx::AcceleratedWidget VersionUpdaterWin::GetElevationParent() {
-  // Look for a visible window belonging to the UI thread.
-  DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  HWND window = NULL;
-  EnumThreadWindows(GetCurrentThreadId(),
-                    WindowEnumeration,
-                    reinterpret_cast<LPARAM>(&window));
-  return window;
+  callback_.Run(FAILED, 0, message);
+#endif
 }
 
 void VersionUpdaterWin::BeginUpdateCheckOnFileThread(bool install_if_newer) {
-  scoped_refptr<base::TaskRunner> task_runner(
-      content::BrowserThread::GetMessageLoopProxyForThread(
-          content::BrowserThread::FILE));
-  BeginUpdateCheck(task_runner, install_if_newer, GetElevationParent(),
-                   base::Bind(&VersionUpdaterWin::OnUpdateCheckResults,
-                              weak_factory_.GetWeakPtr()));
+  BeginUpdateCheck(content::BrowserThread::GetMessageLoopProxyForThread(
+                       content::BrowserThread::FILE),
+                   g_browser_process->GetApplicationLocale(), install_if_newer,
+                   FindVisibleWindow(), weak_factory_.GetWeakPtr());
 }
 
 }  // namespace