Modernize on-demand update checks on Windows.

chrome/browser/google/google_update_win.h

Index: chrome/browser/google/google_update_win.h
diff --git a/chrome/browser/google/google_update_win.h b/chrome/browser/google/google_update_win.h
index 659f3932caebfb087cb169da136583d94f10a1d6..02657e880373fd1b541410526ab4ea77dff975b0 100644
--- a/chrome/browser/google/google_update_win.h
+++ b/chrome/browser/google/google_update_win.h
@@ -5,18 +5,18 @@
 #ifndef CHROME_BROWSER_GOOGLE_GOOGLE_UPDATE_WIN_H_
 #define CHROME_BROWSER_GOOGLE_GOOGLE_UPDATE_WIN_H_
 
+#include <windows.h>
+
 #include "base/basictypes.h"
+#include "base/callback_forward.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string16.h"
+#include "base/win/scoped_comptr.h"
 #include "google_update/google_update_idl.h"
 
 namespace base {
-class MessageLoop;
-}
-
-namespace views {
-class Widget;
-}
+class TaskRunner;
+}  // namespace base
 
 // The status of the upgrade. UPGRADE_STARTED and UPGRADE_CHECK_STARTED are
 // internal states and will not be reported as results to the listener.
@@ -52,10 +52,10 @@ enum GoogleUpdateErrorCode {
   // Google Update OnDemand COM class reported an error during a check for
   // update (or while upgrading).
   GOOGLE_UPDATE_ONDEMAND_CLASS_REPORTED_ERROR = 4,
-  // A call to GetResults failed.
-  GOOGLE_UPDATE_GET_RESULT_CALL_FAILED = 5,
-  // A call to GetVersionInfo failed.
-  GOOGLE_UPDATE_GET_VERSION_INFO_FAILED = 6,
+  // A call to GetResults failed. DEPRECATED.
+  // GOOGLE_UPDATE_GET_RESULT_CALL_FAILED = 5,
+  // A call to GetVersionInfo failed. DEPRECATED
+  // GOOGLE_UPDATE_GET_VERSION_INFO_FAILED = 6,
   // An error occurred while upgrading (or while checking for update).
   // Check the Google Update log in %TEMP% for more details.
   GOOGLE_UPDATE_ERROR_UPDATING = 7,
@@ -68,84 +68,43 @@ enum GoogleUpdateErrorCode {
   NUM_ERROR_CODES
 };
 
-// The GoogleUpdateStatusListener interface is used by components to receive
-// notifications about the results of an Google Update operation.
-class GoogleUpdateStatusListener {
- public:
-  // This function is called when Google Update has finished its operation and
-  // wants to notify us about the results. |results| represents what the end
-  // state is, |error_code| represents what error occurred, |error_message| is a
-  // string version of the same (might be blank) and |version| specifies what
-  // new version Google Update detected (or installed). This value can be a
-  // blank string, if the version tag in the Update{} block (in Google Update's
-  // server config for Chrome) is blank.
-  virtual void OnReportResults(GoogleUpdateUpgradeResult results,
-                               GoogleUpdateErrorCode error_code,
-                               const base::string16& error_message,
-                               const base::string16& version) = 0;
-};
-
-////////////////////////////////////////////////////////////////////////////////
-//
-// The Google Update class is responsible for communicating with Google Update
-// and get it to perform operations on our behalf (for example, CheckForUpdate).
-// This class will report back to its parent via the GoogleUpdateStatusListener
-// interface and will delete itself after reporting back.
-//
-////////////////////////////////////////////////////////////////////////////////
-class GoogleUpdate : public base::RefCountedThreadSafe<GoogleUpdate> {
- public:
-  GoogleUpdate();
-
-  // Ask Google Update to see if a new version is available. If the parameter
-  // |install_if_newer| is true then Google Update will also install that new
-  // version.
-  // |window| should point to a foreground window. This is needed to ensure
-  // that Vista/Windows 7 UAC prompts show up in the foreground. It may also
-  // be null.
-  void CheckForUpdate(bool install_if_newer, HWND window);
-
-  // Pass NULL to clear the listener
-  void set_status_listener(GoogleUpdateStatusListener* listener) {
-    listener_ = listener;
-  }
-
- private:
-  friend class base::RefCountedThreadSafe<GoogleUpdate>;
-
-  virtual ~GoogleUpdate();
-
-  // This function reports failure from the Google Update operation to the
-  // listener.
-  // Note, after this function completes, this object will have deleted itself.
-  bool ReportFailure(HRESULT hr, GoogleUpdateErrorCode error_code,
-                     const base::string16& error_message,
-                     base::MessageLoop* main_loop);
-
-  // The update check needs to run on another thread than the main thread, and
-  // therefore CheckForUpdate will delegate to this function. |main_loop| points
-  // to the message loop that the response must come from.
-  // |window| should point to a foreground window. This is needed to ensure that
-  // Vista/Windows 7 UAC prompts show up in the foreground. It may also be null.
-  void InitiateGoogleUpdateCheck(bool install_if_newer, HWND window,
-                                 base::MessageLoop* main_loop);
-
-  // This function reports the results of the GoogleUpdate operation to the
-  // listener. If results indicates an error, the |error_code| and
-  // |error_message| will indicate which error occurred.
-  // Note, after this function completes, this object will have deleted itself.
-  void ReportResults(GoogleUpdateUpgradeResult results,
-                     GoogleUpdateErrorCode error_code,
-                     const base::string16& error_message);
-
-  // Which version string Google Update found (if a new one was available).
-  // Otherwise, this will be blank.
-  base::string16 version_available_;
-
-  // The listener who is interested in finding out the result of the operation.
-  GoogleUpdateStatusListener* listener_;
-
-  DISALLOW_COPY_AND_ASSIGN(GoogleUpdate);
-};
+// A callback run when a check for updates has completed. |result| indicates the
+// end state of the operation. When |result| is UPGRADE_ERROR, |error_code| and
+// |error_message| indicate the the nature of the error. When |result| is
+// UPGRADE_IS_AVAILABLE or UPGRADE_SUCCESSFUL, |version| may indicate the new
+// version Google Update detected (it may, however, be empty).
+typedef base::Callback<void(GoogleUpdateUpgradeResult result,
+                            GoogleUpdateErrorCode error_code,
+                            const base::string16& error_message,
+                            const base::string16& version)> UpdateCheckCallback;
+
+// Initiates an update check on the FILE thread. If |install_if_newer| is true,
+// an update will be applied. |elevation_window| is the window which should own
+// any necessary elevation UI. |callback| will be run on the caller's thread
+// when the check completes.
+void CheckForUpdate(bool install_if_newer,
+                    HWND elevation_window,
+                    const UpdateCheckCallback& callback);
+
+namespace internal {

[Peter Kasting, 2014/11/26 22:13:05]

We also shouldn't be using an ad-hoc "internal" namespace when we're outside the
chrome namespace.  (Note that if we combine the two CheckForUpdate() methods as
I want to do, the primary reason for this namespace disappears anyway.)

[grt (UTC plus 2), 2014/11/27 04:44:57]

I don't understand this suggestion, nor why it seems ad-hoc to you. The factory
type and factory setter are also exposed only for testing. A consumer of the
function above has no need to see any of this. The options I can think of are:

1. Use the internal namespace as here.
2. Put "Internal" in the names of these things (e.g., CheckForUpdateInternal).
3. Put "for testing only" or something similar in a doc comment.
4. Introduce a new "google_update_internal_win.h" header with these
declarations.

#1 is done a fair amount throughout the codebase (in some cases mixed with #4).
It seems to very clearly group multiple declarations/definitions together in a
"don't touch this" sort of way. I find #2 uglier, especially if it's for
multiple names. #3 is too easy to miss. I like #4, but it seems a bit
heavy-weight for this.

[Peter Kasting, 2014/11/27 08:07:34]

Namespace usage in Chrome has been discussed on the mailing lists several times,
not always clearly, and the general conclusion has been that we mostly ban
namespaces that don't correspond to top-level modules.  This is what led to e.g.
the "chrome::" namespace being banned.

If other code in Chrome is using "internal" namespaces, that code is likely
wrong per those discussions, and should be fixed.  It's not a big enough deal to
go around trying to wipe it out, but I don't want to introduce more cases.
As I've said several times, I really really really think the best way to go here
is to combine the two CheckForUpdate() methods.  At that point you're down to
one remaining method that's only needed by tests.  I don't think it's terrible
for that to just live alongside the other lone method in this file.
See above.

Plus, "internal" is meaningless.  It doesn't mean "for testing", it just means
"don't touch me".  We can do better than that if we really want to tell callers
something about this stuff.
I suggested something like this as a fallback in another comment, but with a
better name.
You already do this (and IMO it's sufficient)
Too heavyweight.

There's a general theme I'm trying to convey in this .h and .cc: I think you're
overdesigning.  You're introducing too many methods, too many namespaces, too
many abstractions.  Yes, all of those abstractions separate implementations from
each other and black box things and so forth, but this code is simple enough
that all that rigor is not really necessary for maintainability, and at that
point it just gets in the way of readability and simplicity.  Please, just make
these files as short and simple as possible.  In a case like this that pays off
more than any of the other benefits.

+
+// A type of callback supplied by tests to provide a custom IGoogleUpdate
+// implementation.
+typedef base::Callback<HRESULT(base::win::ScopedComPtr<IGoogleUpdate>*)>
+    OnDemandAppsClassFactory;
+
+// For use by tests that wish to provide a custom IGoogleUpdate implementation
+// independent of Google Update's.
+void SetGoogleUpdateFactory(
+    const OnDemandAppsClassFactory& google_update_factory);
+
+// Runs the update check described in CheckForUpdate above on |task_runner|.
+// Exposed for unit testing.
+void CheckForUpdate(const scoped_refptr<base::TaskRunner>& task_runner,
+                    bool install_if_newer,
+                    HWND elevation_window,
+                    const UpdateCheckCallback& callback);
+
+}  // namespace internal
 
 #endif  // CHROME_BROWSER_GOOGLE_GOOGLE_UPDATE_WIN_H_