Extract Profile-independent GCMService from GCMProfileService

chrome/browser/services/gcm/gcm_service.h

Index: chrome/browser/services/gcm/gcm_service.h
diff --git a/chrome/browser/services/gcm/gcm_profile_service.h b/chrome/browser/services/gcm/gcm_service.h
similarity index 63%
copy from chrome/browser/services/gcm/gcm_profile_service.h
copy to chrome/browser/services/gcm/gcm_service.h
index 3817b0ea8f0f5076c01f1dfd07bed6724cbb2ee8..ba6f4000a6fc60c76e54a364118e049fdf792f1f 100644
--- a/chrome/browser/services/gcm/gcm_profile_service.h
+++ b/chrome/browser/services/gcm/gcm_service.h
@@ -1,50 +1,40 @@
-// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Copyright 2014 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.
 
-#ifndef CHROME_BROWSER_SERVICES_GCM_GCM_PROFILE_SERVICE_H_
-#define CHROME_BROWSER_SERVICES_GCM_GCM_PROFILE_SERVICE_H_
+#ifndef CHROME_BROWSER_SERVICES_GCM_GCM_SERVICE_H_
+#define CHROME_BROWSER_SERVICES_GCM_GCM_SERVICE_H_
 
 #include <map>
 #include <string>
+#include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/compiler_specific.h"
-#include "base/gtest_prod_util.h"
+#include "base/files/file_path.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
 #include "base/memory/weak_ptr.h"
+#include "chrome/browser/invalidation/invalidation_auth_provider.h"
 #include "chrome/browser/services/gcm/default_gcm_app_handler.h"
-#include "components/keyed_service/core/keyed_service.h"
-#include "components/signin/core/browser/signin_manager_base.h"
-#include "content/public/browser/notification_observer.h"
-#include "content/public/browser/notification_registrar.h"
 #include "google_apis/gcm/gcm_client.h"
 
-class Profile;
-
-namespace base {
-class Value;
-}
-
 namespace extensions {
 class ExtensionGCMAppHandlerTest;
 }
 
-namespace user_prefs {
-class PrefRegistrySyncable;
+namespace net {
+class URLRequestContextGetter;
 }
 
 namespace gcm {
 
 class GCMAppHandler;
 class GCMClientFactory;
-class GCMProfileServiceTestConsumer;
 
-// Acts as a bridge between GCM API and GCMClient layer. It is profile based.
-class GCMProfileService : public KeyedService,
-                          public content::NotificationObserver,
-                          public SigninManagerBase::Observer {
+// A bridge between the GCM users in Chrome and the GCMClient layer.
+class GCMService : public invalidation::InvalidationAuthProvider::Observer {
  public:
   typedef base::Callback<void(const std::string& registration_id,
                               GCMClient::Result result)> RegisterCallback;
@@ -54,29 +44,10 @@ class GCMProfileService : public KeyedService,
   typedef base::Callback<void(const GCMClient::GCMStatistics& stats)>
       RequestGCMStatisticsCallback;
 
-  // Any change made to this enum should have corresponding change in the
-  // GetGCMEnabledStateString(...) function.
-  enum GCMEnabledState {
-    // GCM is always enabled. GCMClient will always load and connect with GCM.
-    ALWAYS_ENABLED,
-    // GCM is only enabled for apps. GCMClient will start to load and connect
-    // with GCM only when GCM API is used.
-    ENABLED_FOR_APPS,
-    // GCM is always disabled. GCMClient will never load and connect with GCM.
-    ALWAYS_DISABLED
-  };
-
-  // Returns the GCM enabled state.
-  static GCMEnabledState GetGCMEnabledState(Profile* profile);
-
-  // Returns text representation of a GCMEnabledState enum entry.
-  static std::string GetGCMEnabledStateString(GCMEnabledState state);
-
-  // Register profile-specific prefs for GCM.
-  static void RegisterProfilePrefs(user_prefs::PrefRegistrySyncable* registry);
-
-  explicit GCMProfileService(Profile* profile);
-  virtual ~GCMProfileService();
+  GCMService(scoped_ptr<invalidation::InvalidationAuthProvider> auth_provider,
+             const scoped_refptr<net::URLRequestContextGetter>& request_context,
+             const base::FilePath& store_path);
+  virtual ~GCMService();
 
   void Initialize(scoped_ptr<GCMClientFactory> gcm_client_factory);
 
@@ -84,14 +55,13 @@ class GCMProfileService : public KeyedService,
 
   void Stop();
 
-  // KeyedService implementation.
-  virtual void Shutdown() OVERRIDE;
+  void Shutdown();
 
   // Adds a handler for a given app.
-  virtual void AddAppHandler(const std::string& app_id, GCMAppHandler* handler);
+  void AddAppHandler(const std::string& app_id, GCMAppHandler* handler);
 
   // Remove the handler for a given app.
-  virtual void RemoveAppHandler(const std::string& app_id);
+  void RemoveAppHandler(const std::string& app_id);
 
   // Registers |sender_id| for an app. A registration ID will be returned by
   // the GCM server.
@@ -123,8 +93,8 @@ class GCMProfileService : public KeyedService,
   // For testing purpose.
   GCMClient* GetGCMClientForTesting() const;
 
-  // Returns the user name if the profile is signed in.
-  std::string SignedInUserName() const;
+  // Returns true if the service was started.
+  bool IsStarted() const;
 
   // Returns true if the gcm client is ready.
   bool IsGCMClientReady() const;
@@ -133,8 +103,20 @@ class GCMProfileService : public KeyedService,
   // then stats won't be modified.
   void RequestGCMStatistics(RequestGCMStatisticsCallback callback);
 
+  // InvalidationAuthProvider::Observer:

[jianli, 2014/04/10 20:39:56]

This seems to be app specific logic and it should not live in
GCMService/GCMProfileService. It should be put within the AppHandler for the
invalidation.

[bartfab (slow), 2014/04/11 16:58:52]

Before this CL, GCMProfileService was an SigninManagerBase::Observer and
received GoogleSigninSucceeded() / GoogleSignedOut() notifications.

With this CL, GCMService no longer has direct access to SigninManagerBase. The
same notifications need to be forwarded by an intermediary class now.

The intermediary that I used was originally written for invalidation, which is
why it was called InvalidationAuthProvider and invoked methods that have
"Invalidation" in their name. I see that this led to confusion. To address this,
I renamed the intermediary class to IdentityProvider (because it just provides
access to GAIA identity details and tokens - it has absolutely nothing to do
with invalidation). This renaming is CL 235273002. With that, the methods are
called OnLogin() / OnLogout() now and are the direct equivalents of
GoogleSigninSucceeded() / GoogleSignedOut() that GCMProfileService used before.

+  virtual void OnInvalidationAuthLogin() OVERRIDE;
+  virtual void OnInvalidationAuthLogout() OVERRIDE;
+
+ protected:
+  virtual bool IsAlwaysEnabled() = 0;

[jianli, 2014/04/10 20:39:56]

This is kind of confusing. You should move GetGCMEnabledState to this base class
and let it return ALWAYS_ENABLED.

However, I do not think you should bypass the GCM enabling/disabling logic for
CrOS device-level invalidation. The disabling logic is very useful to deploy the
kill to GCM connection in case something goes wrong.

[bartfab (slow), 2014/04/11 16:58:52]

The reason I implemented it like this is that for the generic base class, there
is only a distinction between always enabled and not always enabled. The more
granular distinction between positive/negative/neutral signals only comes into
play when a GCMService is tied to a profile. The device-level GCMService will
not need this level of control.

And yes, of course the device-level GCMService will have logic to disable it.
The signal will just be different - instead of a profile pref, it will be a
local state pref.

+
+  // Resets the GCMClient instance.
+  void ResetGCMClient();
+
+  scoped_ptr<invalidation::InvalidationAuthProvider> auth_provider_;

[jianli, 2014/04/10 20:39:56]

This should not live here. GCMService should not have knowledge about
invalidation. It should be factored into AppHandler.

[bartfab (slow), 2014/04/11 16:58:52]

As above, sorry for the confusion. This is just an interface for accessing GAIA
sign-in state. I renamed the class to IdentityProvider to make it clear that it
has absolutely nothing to do with invalidation.

+
  private:
-  friend class GCMProfileServiceTestConsumer;
+  friend class TestGCMServiceWrapper;
   friend class extensions::ExtensionGCMAppHandlerTest;
 
   class DelayedTaskController;
@@ -142,31 +124,16 @@ class GCMProfileService : public KeyedService,
 
   typedef std::map<std::string, GCMAppHandler*> GCMAppHandlerMap;
 
-  // Overridden from content::NotificationObserver:
-  virtual void Observe(int type,
-                       const content::NotificationSource& source,
-                       const content::NotificationDetails& details) OVERRIDE;
-
-  // Overridden from SigninManagerBase::Observer:
-  virtual void GoogleSigninSucceeded(const std::string& username,
-                                     const std::string& password) OVERRIDE;
-  virtual void GoogleSignedOut(const std::string& username) OVERRIDE;
-
-  // Ensures that the GCMClient is loaded and the GCM check-in is done when
-  // the profile was signed in.
+  // Ensures that the GCMClient is loaded and the GCM check-in is done if the
+  // |auth_provider_| is able to supply an account ID.
   void EnsureLoaded();
 
   // Remove cached data when GCM service is stopped.
   void RemoveCachedData();
 
-  // Checks out of GCM when the profile has been signed out. This will erase
-  // all the cached and persisted data.
+  // Checks out of GCM and erases any cached and persisted data.
   void CheckOut();
 
-  // Resets the GCMClient instance. This is called when the profile is being
-  // destroyed.
-  void ResetGCMClient();
-
   // Ensures that the app is ready for GCM functions and events.
   GCMClient::Result EnsureAppReady(const std::string& app_id);
 
@@ -201,21 +168,22 @@ class GCMProfileService : public KeyedService,
 
   void RequestGCMStatisticsFinished(GCMClient::GCMStatistics stats);
 
-  // The profile which owns this object.
-  Profile* profile_;
+  scoped_refptr<net::URLRequestContextGetter> request_context_;

[jianli, 2014/04/10 20:39:56]

This should be obtained by calling virtual method GetURLRequestContextGetter.

[bartfab (slow), 2014/04/11 16:58:52]

Done.

+
+  const base::FilePath store_path_;

[jianli, 2014/04/10 20:39:56]

ditto.

[bartfab (slow), 2014/04/11 16:58:52]

Done.

 
   // Flag to indicate if GCMClient is ready.
   bool gcm_client_ready_;
 
-  // The username of the signed-in profile.
-  std::string username_;
-
-  content::NotificationRegistrar registrar_;
+  // The account ID that this service is responsible for. Empty when the service
+  // is not running.
+  std::string account_id_;
 
   scoped_ptr<DelayedTaskController> delayed_task_controller_;
 
-  // For all the work occured in IO thread.
-  scoped_refptr<IOWorker> io_worker_;
+  // For all the work occurring on the IO thread. Must be destroyed on the IO
+  // thread.
+  scoped_ptr<IOWorker> io_worker_;

[jianli, 2014/04/10 20:39:56]

Why do you change to use scoped_ptr?

[bartfab (slow), 2014/04/11 16:58:52]

To make the ownership explicit. The previous implementation using scoped_refptr
was wrong in two ways:

1) With scoped_refptr, the IOWorker will be destroyed when the last reference to
it is released. Since references are held on different threads, this means that
it is undefined which thread the destructor will run on.

2) The above actually did lead to destruction on the wrong thread in practice:
When GCMProfileService is shutting down (on the UI thread), it happens to
release the last reference to the IOWorker. This causes ~IOWorker() to runn on
the UI thread while the object may actually still be doing something on the IO
thread, causing a race and potential for unallocated memory reads/writes.

 
   // App handler map (from app_id to handler pointer).
   // The handler is not owned.
@@ -237,11 +205,11 @@ class GCMProfileService : public KeyedService,
   RequestGCMStatisticsCallback request_gcm_statistics_callback_;
 
   // Used to pass a weak pointer to the IO worker.
-  base::WeakPtrFactory<GCMProfileService> weak_ptr_factory_;
+  base::WeakPtrFactory<GCMService> weak_ptr_factory_;
 
-  DISALLOW_COPY_AND_ASSIGN(GCMProfileService);
+  DISALLOW_COPY_AND_ASSIGN(GCMService);
 };
 
 }  // namespace gcm
 
-#endif  // CHROME_BROWSER_SERVICES_GCM_GCM_PROFILE_SERVICE_H_
+#endif  // CHROME_BROWSER_SERVICES_GCM_GCM_SERVICE_H_