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..c8211472a2da83df38bb262b54fef6c09dce5a15 100644
--- a/chrome/browser/ui/webui/help/version_updater_win.cc
+++ b/chrome/browser/ui/webui/help/version_updater_win.cc
@@ -2,6 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <algorithm>
+
 #include "base/memory/ref_counted.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
@@ -9,6 +11,7 @@
 #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/google/google_update_win.h"
 #include "chrome/browser/lifetime/application_lifetime.h"
 #include "chrome/browser/ui/browser.h"
@@ -44,12 +47,14 @@ class VersionUpdaterWin : public VersionUpdater {
 
   // chrome::UpdateCheckCallback.
   void OnUpdateCheckResults(GoogleUpdateUpgradeResult result,
+                            double progress,
                             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,
+                    double update_progress,
                     GoogleUpdateErrorCode error_code,
                     const base::string16& error_message);
 
@@ -132,7 +137,7 @@ 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,
+    UpdateStatus(UPGRADE_CHECK_STARTED, 0.0, GOOGLE_UPDATE_NO_ERROR,
                  base::string16());
     // Specify false to not upgrade yet.
     BeginUpdateCheckOnFileThread(false);
@@ -145,13 +150,15 @@ void VersionUpdaterWin::RelaunchBrowser() const {
 
 void VersionUpdaterWin::OnUpdateCheckResults(
     GoogleUpdateUpgradeResult result,
+    double progress,
     GoogleUpdateErrorCode error_code,
     const base::string16& error_message,
     const base::string16& version) {
-  UpdateStatus(result, error_code, error_message);
+  UpdateStatus(result, progress, error_code, error_message);
 }
 
 void VersionUpdaterWin::UpdateStatus(GoogleUpdateUpgradeResult result,
+                                     double update_progress,
                                      GoogleUpdateErrorCode error_code,
                                      const base::string16& error_message) {
   // For Chromium builds it would show an error message.
@@ -159,6 +166,7 @@ void VersionUpdaterWin::UpdateStatus(GoogleUpdateUpgradeResult result,
   // just the update server is not available for non-official builds.
 #if defined(GOOGLE_CHROME_BUILD)
   Status status = UPDATED;
+  int progress = 0;
   base::string16 message;
 
   switch (result) {
@@ -168,10 +176,13 @@ void VersionUpdaterWin::UpdateStatus(GoogleUpdateUpgradeResult result,
     }
     case UPGRADE_STARTED: {
       status = UPDATING;
+      progress = static_cast<int>((update_progress * 100.0) + 0.5);

[Peter Kasting, 2015/05/07 01:12:28]

Technically, things like this should probably use ToFlooredInt() from
safe_integer_conversions.h.

However, looking at the code here to convert this double to an int, and the code
in UpdateCheckDriver::IsIntermediateState() that actually computes the value, I
wonder whether making this a double is really the right thing to do.

It seems like we could use an int everywhere and save a bit of complexity here,
while not really making the code there worse: the "downloading" case basically
changes from "divide by 2" to "multiply by 50" and the "installing" case just
becomes "50 + install_progress / 2".

It doesn't really seem more arbitrary to me to pass an int ranging from 0-100
than a double ranging from 0-1.  WDYT?

(I'd probably just ditch the min() call too, this code is tightly tied to the
other code and it seems OK to rely on that code giving back a valid number.  If
there is a clamp needed anywhere, I'd suggest it in the "installing" case that
fetches the original LONG that's from 0-100 -- if one is even needed there. 
This would allow ditching the <algorithm> include.)

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

Awesome, I didn't know that was a thing.
SGTM. I was inspired to use a double by the CrOS case, but I don't think there's
any compelling reason to use it here.
No longer needed.

+      progress = std::min(progress, 100);
       break;
     }
     case UPGRADE_IS_AVAILABLE: {
-      UpdateStatus(UPGRADE_STARTED, GOOGLE_UPDATE_NO_ERROR, base::string16());
+      UpdateStatus(UPGRADE_STARTED, 0.0, GOOGLE_UPDATE_NO_ERROR,
+                   base::string16());
       // Specify true to upgrade now.
       BeginUpdateCheckOnFileThread(true);
       return;
@@ -192,13 +203,11 @@ void VersionUpdaterWin::UpdateStatus(GoogleUpdateUpgradeResult result,
     }
     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 {
+      // Google Update provides a nice message for the policy case. Use this
+      // generic error for the policy case only if no message from Google Update
+      // is missing.

[Peter Kasting, 2015/05/07 01:12:28]

"if no message is missing"?  Is there a double negative there?

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

Done.

+      if (error_code != GOOGLE_UPDATE_DISABLED_BY_POLICY ||
+          error_message.empty()) {
         message =
             l10n_util::GetStringFUTF16Int(IDS_UPGRADE_ERROR, error_code);
       }
@@ -212,9 +221,7 @@ void VersionUpdaterWin::UpdateStatus(GoogleUpdateUpgradeResult result,
     }
   }
 
-  // TODO(mad): Get proper progress value instead of passing 0.
-  // http://crbug.com/136117
-  callback_.Run(status, 0, message);
+  callback_.Run(status, progress, message);
 #endif  //  defined(GOOGLE_CHROME_BUILD)
 }
 
@@ -260,7 +267,8 @@ 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(),
+  BeginUpdateCheck(task_runner, g_browser_process->GetApplicationLocale(),
+                   install_if_newer, GetElevationParent(),
                    base::Bind(&VersionUpdaterWin::OnUpdateCheckResults,
                               weak_factory_.GetWeakPtr()));
 }