Refactor AO2TS to make it easier to componentize.

google_apis/gaia/oauth2_token_service.h

Index: google_apis/gaia/oauth2_token_service.h
diff --git a/google_apis/gaia/oauth2_token_service.h b/google_apis/gaia/oauth2_token_service.h
index 4decbf4c2a0eb48af455ed0b334fdd0e9c0c1cc8..879570f469c3468aeb8a16e99caf9a6686c85e83 100644
--- a/google_apis/gaia/oauth2_token_service.h
+++ b/google_apis/gaia/oauth2_token_service.h
@@ -26,6 +26,7 @@ class URLRequestContextGetter;
 
 class GoogleServiceAuthError;
 class OAuth2AccessTokenFetcher;
+class OAuth2TokenServiceDelegate;
 
 // Abstract base class for a service that fetches and caches OAuth2 access
 // tokens. Concrete subclasses should implement GetRefreshToken to return
@@ -129,7 +130,7 @@ class OAuth2TokenService : public base::NonThreadSafe {
                                 const ScopeSet& scopes) = 0;
   };
 
-  OAuth2TokenService();
+  OAuth2TokenService(OAuth2TokenServiceDelegate* delegate);

[xiyuan, 2015/06/26 18:15:53]

nit: explicit and use scoped_ptr to make ownership transfer more clear,

i.e.
  explicit OAuth2TokenService(scoped_ptr<OAuth2TokenServiceDelegate> delegate);

[gogerald1, 2015/07/01 17:58:41]

Acknowledged.

   virtual ~OAuth2TokenService();
 
   // Add or remove observers of this token service.
@@ -172,31 +173,30 @@ class OAuth2TokenService : public base::NonThreadSafe {
 
   // Lists account IDs of all accounts with a refresh token maintained by this
   // instance.
-  virtual std::vector<std::string> GetAccounts();
+  std::vector<std::string> GetAccounts() const;
 
   // Returns true if a refresh token exists for |account_id|. If false, calls to
   // |StartRequest| will result in a Consumer::OnGetTokenFailure callback.
-  virtual bool RefreshTokenIsAvailable(const std::string& account_id) const = 0;
+  bool RefreshTokenIsAvailable(const std::string& account_id) const;
+
+  void RevokeAllCredentials();

[Mattias Nissler (ping if slow), 2015/06/29 08:20:06]

Would be good to put a comment stating what this does. Drop all access tokens?
Drop even refresh tokens?

[gogerald1, 2015/07/01 17:58:41]

Done.

 
   // Mark an OAuth2 |access_token| issued for |account_id| and |scopes| as
   // invalid. This should be done if the token was received from this class,
   // but was not accepted by the server (e.g., the server returned
   // 401 Unauthorized). The token will be removed from the cache for the given
   // scopes.
-  void InvalidateToken(const std::string& account_id,
-                       const ScopeSet& scopes,
-                       const std::string& access_token);
+  void InvalidateAccessToken(const std::string& account_id,
+                             const ScopeSet& scopes,
+                             const std::string& access_token);
 
   // Like |InvalidateToken| except is uses |client_id| to identity OAuth2 client
   // app that issued the request instead of Chrome's default values.
-  void InvalidateTokenForClient(const std::string& account_id,
-                                const std::string& client_id,
-                                const ScopeSet& scopes,
-                                const std::string& access_token);
+  void InvalidateAccessTokenForClient(const std::string& account_id,
+                                      const std::string& client_id,
+                                      const ScopeSet& scopes,
+                                      const std::string& access_token);
 
-
-  // Return the current number of entries in the cache.
-  int cache_size_for_testing() const;
   void set_max_authorization_token_fetch_retries_for_testing(int max_retries);
   // Returns the current number of pending fetchers matching given params.
   size_t GetNumPendingRequestsForTesting(
@@ -204,6 +204,8 @@ class OAuth2TokenService : public base::NonThreadSafe {
       const std::string& account_id,
       const ScopeSet& scopes) const;
 
+  OAuth2TokenServiceDelegate* GetDelegate();

[Mattias Nissler (ping if slow), 2015/06/29 08:20:06]

From an architectural perspective, it seems wrong to have this function in the
public interface. The CL description suggests that the delegate makes it easier
to implement the platform-specific logic for token service implementations
separate from OAuth2TokenService. I thus expected that the delegate is merely an
implementation detail of O2TS (and subclasses) that consumers wouldn't need to
know about. However, this function means that all the complexity of having to
deal with a delegate is exposed, and in fact some code does make calls directly
into the delegate.

Roger, what are your thoughts regarding this?

[msarda, 2015/06/29 08:49:44]

I agree that from architectural perspective having the delegate public may look
strange. However, on iOS we have specific logic in PO2TS_IOS to reload
credentials. That logic is now being moved to the delegate. So if we do not want
any of that logic in the token service and we do not want to subclass the PO2TS
any more, then we need this delegate to be public.

[Mattias Nissler (ping if slow), 2015/06/29 09:01:29]

Who calls that logic to reload credentials? If that's an action which is
relevant for consumers in general, it may make sense to lift the API into PO2TS.
If the call comes from IOS-specific code, it may make sense for that code to
hold on to the delegate pointer that it passed to PO2TS at construction time so
it can make the call without relying on the getter here. I don't know your use
case though, so these suggestions are merely a stab in the dark.

[Roger Tawa OOO till Jul 10th, 2015/06/30 19:55:30]

There are a few types of token services in chrome, depending on platform.  On
desktop, the token service implementation does it all.  It mints and caches
access tokens, manages refresh tokens/accounts, and updates the access tokens
from refresh tokens.

On mobile, the token service implementation is a thin wrapper around non-chrome
services.  In these cases, the token service mints and caches access tokens but
has no way to manipulate refresh tokens (i.e., add and/or remove accounts from
the device).  The delegate updates access tokens when needed, as that is
platform specific.  Furthermore, there are other mobile specific things that
need to be done, and that code now belongs to the delegate as well.

Cros is slightly different.  The token service always has at least one refresh
token matching the signed in user.  This token cannot be removed.  But when
account consistency is enabled, cros can manager secondary accounts.

This CL keeps the interface for requesting access tokens and the caching of
these tokens in the OAuth2TokenService class.  The delegate handles updating the
access tokens from the refresh tokens.  The management of refresh tokens is also
done by the delegate if needed.  There is no longer a need to have
OAuth2TokenService derivative classes per platform.

An alternative could be to save the delegate pointer somewhere and then get it
directly, as you suggest.  Maybe the delegate should even be its own PKS.  But I
don't think it's a problem for the token service to have a public GetDelegate()
method.  The token service is always backed by some delegate.

If you feel strongly about this, I could open a bug to track how to better
manage the token service delegates as a follow up CL.  This CL is already large
enough.  Is that OK?

[Mattias Nissler (ping if slow), 2015/06/30 20:42:51]

That's awesome, and I'm very happy to hear this. Does that mean the plan is to
remove all OAuth2TokenService derivatives in follow-up CLs?
As long as regular consumers (i.e. services that just request access tokens)
don't have to be aware of the fact that there's a delegate, I'm fine with
keeping the public GetDelegate() function. My main concern was to avoid making
the OAuth2TokenService API more complex to use for consumers.

In that spirit, I suggest to simplify FakeOAuth2TokenService to provide
functions that allow consumer to set up fake tokens? That way,
FakeOAuth2TokenService consumers don't need to be aware of the presence of the
delegate, and in fact we may change the interface between OAuth2TokenService and
OAuth2TokenServiceDelegate with less friction (i.e. tests to update) in the
future.
I trust you to make a good decision on this architectural question as you know
this code much better than I do. I was just trying to point out what looked like
an architectural flaw from a perspective of OAuth2TokenService consumers. Please
don't consider this comment thread informative, i.e. I'm not meaning to hold the
CL over it.

[gogerald1, 2015/07/01 17:58:41]

Acknowledged.

+
  protected:
   // Implements a cancelable |OAuth2TokenService::Request|, which should be
   // operated on the UI thread.
@@ -232,20 +234,9 @@ class OAuth2TokenService : public base::NonThreadSafe {
     Consumer* const consumer_;
   };
 
-  // Helper class to scope batch changes.
-  class ScopedBatchChange {
-   public:
-    explicit ScopedBatchChange(OAuth2TokenService* token_service);
-    ~ScopedBatchChange();
-   private:
-    OAuth2TokenService* token_service_;  // Weak.
-    DISALLOW_COPY_AND_ASSIGN(ScopedBatchChange);
-  };
-
-  // Subclasses can override if they want to report errors to the user.
-  virtual void UpdateAuthError(
-      const std::string& account_id,
-      const GoogleServiceAuthError& error);
+  // Implement it in delegates if they want to report errors to the user.
+  void UpdateAuthError(const std::string& account_id,
+                       const GoogleServiceAuthError& error);
 
   // Add a new entry to the cache.
   // Subclasses can override if there are implementation-specific reasons
@@ -270,16 +261,8 @@ class OAuth2TokenService : public base::NonThreadSafe {
   // Cancels all requests related to a given |account_id|.
   void CancelRequestsForAccount(const std::string& account_id);
 
-  // Called by subclasses to notify observers.
-  virtual void FireRefreshTokenAvailable(const std::string& account_id);
-  virtual void FireRefreshTokenRevoked(const std::string& account_id);
-  virtual void FireRefreshTokensLoaded();
-
-  virtual void StartBatchChanges();
-  virtual void EndBatchChanges();
-
   // Fetches an OAuth token for the specified client/scopes. Virtual so it can
-  // be overridden for tests and for platform-specific behavior on Android.
+  // be overridden for tests and for platform-specific behavior.
   virtual void FetchOAuth2Token(RequestImpl* request,
                                 const std::string& account_id,
                                 net::URLRequestContextGetter* getter,
@@ -287,27 +270,24 @@ class OAuth2TokenService : public base::NonThreadSafe {
                                 const std::string& client_secret,
                                 const ScopeSet& scopes);
 
-  // Creates an access token fetcher for the given account id.
-  //
-  // Subclasses should override to create an access token fetcher for the given
-  // |account_id|. This method is only called if subclasses use the default
-  // implementation of |FetchOAuth2Token|.
-  virtual OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(
+  // Create an access token fetcher for the given account id.
+  OAuth2AccessTokenFetcher* CreateAccessTokenFetcher(
       const std::string& account_id,
       net::URLRequestContextGetter* getter,
-      OAuth2AccessTokenConsumer* consumer) = 0;
+      OAuth2AccessTokenConsumer* consumer);
 
   // Invalidates the |access_token| issued for |account_id|, |client_id| and
   // |scopes|. Virtual so it can be overriden for tests and for platform-
   // specifc behavior.
-  virtual void InvalidateOAuth2Token(const std::string& account_id,
-                                     const std::string& client_id,
-                                     const ScopeSet& scopes,
-                                     const std::string& access_token);
+  virtual void InvalidateAccessTokenImpl(const std::string& account_id,
+                                         const std::string& client_id,
+                                         const ScopeSet& scopes,
+                                         const std::string& access_token);
 
  private:
   class Fetcher;
   friend class Fetcher;
+  friend class OAuth2TokenServiceDelegate;
 
   // The parameters used to fetch an OAuth2 access token.
   struct RequestParameters {
@@ -327,9 +307,9 @@ class OAuth2TokenService : public base::NonThreadSafe {
 
   typedef std::map<RequestParameters, Fetcher*> PendingFetcherMap;
 
-  // Derived classes must provide a request context used for fetching access
-  // tokens with the |StartRequest| method.
-  virtual net::URLRequestContextGetter* GetRequestContext() = 0;
+  // Provide a request context used for fetching access tokens with the
+  // |StartRequest| method.
+  net::URLRequestContextGetter* GetRequestContext() const;
 
   // Struct that contains the information of an OAuth2 access token.
   struct CacheEntry {
@@ -379,14 +359,12 @@ class OAuth2TokenService : public base::NonThreadSafe {
   typedef std::map<RequestParameters, CacheEntry> TokenCache;
   TokenCache token_cache_;
 
+  scoped_ptr<OAuth2TokenServiceDelegate> delegate_;
+
   // A map from fetch parameters to a fetcher that is fetching an OAuth2 access
   // token using these parameters.
   PendingFetcherMap pending_fetchers_;
 
-  // List of observers to notify when refresh token availability changes.
-  // Makes sure list is empty on destruction.
-  base::ObserverList<Observer, true> observer_list_;
-
   // List of observers to notify when access token status changes.
   base::ObserverList<DiagnosticsObserver, true> diagnostics_observer_list_;
 
@@ -399,6 +377,7 @@ class OAuth2TokenService : public base::NonThreadSafe {
   FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest, RequestParametersOrderTest);
   FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest,
                            SameScopesRequestedForDifferentClients);
+  FRIEND_TEST_ALL_PREFIXES(OAuth2TokenServiceTest, UpdateClearsCache);
 
   DISALLOW_COPY_AND_ASSIGN(OAuth2TokenService);
 };