Refactor AO2TS to make it easier to componentize.

chrome/browser/chromeos/settings/device_oauth2_token_service.cc

Index: chrome/browser/chromeos/settings/device_oauth2_token_service.cc
diff --git a/chrome/browser/chromeos/settings/device_oauth2_token_service.cc b/chrome/browser/chromeos/settings/device_oauth2_token_service.cc
index 1dc6bde934c3f50fbc53cff2d0e33b5f09c50b46..49c145d4126ce3968c4aa6d286c37d6a350ae6c8 100644
--- a/chrome/browser/chromeos/settings/device_oauth2_token_service.cc
+++ b/chrome/browser/chromeos/settings/device_oauth2_token_service.cc
@@ -43,25 +43,79 @@ struct DeviceOAuth2TokenService::PendingRequest {
   const ScopeSet scopes;
 };
 
+DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::

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

It's pretty odd that the CL introduces a delegate interface that the code here
then implements trivially to delegate back to DeviceOAuth2TokenService. So in a
sense, DeviceOAuth2TokenService is now its own delegate, which seems a bit
silly. Is there a longer-term plan for this that I'm missing?

+    DeviceOAuth2TokenServiceDelegate(
+        DeviceOAuth2TokenService* device_token_service)
+    : device_token_service_(device_token_service) {
+}
+
+DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::
+    ~DeviceOAuth2TokenServiceDelegate() {
+}
+
+OAuth2AccessTokenFetcher* DeviceOAuth2TokenService::
+    DeviceOAuth2TokenServiceDelegate::CreateAccessTokenFetcher(
+        const std::string& account_id,
+        net::URLRequestContextGetter* getter,
+        OAuth2AccessTokenConsumer* consumer) {
+  return device_token_service_->CreateAccessTokenFetcherImpl(account_id, getter,
+                                                             consumer);
+}
+
+bool DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::
+    RefreshTokenIsAvailable(const std::string& account_id) const {
+  return device_token_service_->RefreshTokenIsAvailableImpl(account_id);
+}
+
+net::URLRequestContextGetter*
+DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::GetRequestContext()
+    const {
+  return device_token_service_->GetRequestContextImpl();
+}
+
+void DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::
+    UpdateAuthError(const std::string& account_id,
+                    const GoogleServiceAuthError& error) {

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

Should have a comment explaining why it's OK that this is empty.

+}
+
+std::vector<std::string>
+DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::GetAccounts() {
+  std::vector<std::string> accounts;
+  std::string account = device_token_service_->GetRobotAccountId();
+  if (!account.empty())
+    accounts.push_back(account);
+  return accounts;
+}
+
+void DeviceOAuth2TokenService::DeviceOAuth2TokenServiceDelegate::
+    RevokeAllCredentials() {

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

Again, please explain why it's OK that this is empty.

+}
+
 void DeviceOAuth2TokenService::OnServiceAccountIdentityChanged() {
   if (!GetRobotAccountId().empty() && !refresh_token_.empty())
-    FireRefreshTokenAvailable(GetRobotAccountId());
+    device_oauth2_token_service_delegate_->FireRefreshTokenAvailable(
+        GetRobotAccountId());
 }
 
 DeviceOAuth2TokenService::DeviceOAuth2TokenService(
     net::URLRequestContextGetter* getter,
     PrefService* local_state)
-    : url_request_context_getter_(getter),
+    : OAuth2TokenService(new DeviceOAuth2TokenServiceDelegate(this)),
+      url_request_context_getter_(getter),
       local_state_(local_state),
       state_(STATE_LOADING),
       max_refresh_token_validation_retries_(3),
       service_account_identity_subscription_(
-          CrosSettings::Get()->AddSettingsObserver(
-              kServiceAccountIdentity,
-              base::Bind(
-                  &DeviceOAuth2TokenService::OnServiceAccountIdentityChanged,
-                  base::Unretained(this))).Pass()),
+          CrosSettings::Get()
+              ->AddSettingsObserver(
+                    kServiceAccountIdentity,
+                    base::Bind(&DeviceOAuth2TokenService::
+                                   OnServiceAccountIdentityChanged,
+                               base::Unretained(this)))
+              .Pass()),
       weak_ptr_factory_(this) {
+  device_oauth2_token_service_delegate_ =
+      (DeviceOAuth2TokenServiceDelegate*)GetDelegate();

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

nit: static_cast<DeviceOAuth2TokenServiceDelegate*>(GetDelegate());

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

Actually, it looks like none of the uses of
|device_oauth2_token_service_delegate_| requires the specific subtype, i.e.
DeviceOAuth2TokenServiceDelegate. If so, does it make sense at all to keep a
pointer of type DeviceOAuth2TokenServiceDelegate?

   // Pull in the system salt.
   SystemSaltGetter::Get()->GetSystemSalt(
       base::Bind(&DeviceOAuth2TokenService::DidGetSystemSalt,
@@ -92,7 +146,8 @@ void DeviceOAuth2TokenService::SetAndSaveRefreshToken(
   // will be done from OnServiceAccountIdentityChanged() once the robot account
   // ID becomes available as well.
   if (!GetRobotAccountId().empty())
-    FireRefreshTokenAvailable(GetRobotAccountId());
+    device_oauth2_token_service_delegate_->FireRefreshTokenAvailable(
+        GetRobotAccountId());
 
   token_save_callbacks_.push_back(result_callback);
   if (!waiting_for_salt) {
@@ -103,7 +158,7 @@ void DeviceOAuth2TokenService::SetAndSaveRefreshToken(
   }
 }
 
-bool DeviceOAuth2TokenService::RefreshTokenIsAvailable(
+bool DeviceOAuth2TokenService::RefreshTokenIsAvailableImpl(
     const std::string& account_id) const {
   switch (state_) {
     case STATE_NO_TOKEN:
@@ -181,7 +236,8 @@ std::string DeviceOAuth2TokenService::GetRefreshToken(
   return std::string();
 }
 
-net::URLRequestContextGetter* DeviceOAuth2TokenService::GetRequestContext() {
+net::URLRequestContextGetter*
+DeviceOAuth2TokenService::GetRequestContextImpl() {
   return url_request_context_getter_.get();
 }
 
@@ -219,7 +275,8 @@ void DeviceOAuth2TokenService::FetchOAuth2Token(
   NOTREACHED() << "Unexpected state " << state_;
 }
 
-OAuth2AccessTokenFetcher* DeviceOAuth2TokenService::CreateAccessTokenFetcher(
+OAuth2AccessTokenFetcher*
+DeviceOAuth2TokenService::CreateAccessTokenFetcherImpl(
     const std::string& account_id,
     net::URLRequestContextGetter* getter,
     OAuth2AccessTokenConsumer* consumer) {
@@ -238,14 +295,14 @@ void DeviceOAuth2TokenService::DidGetSystemSalt(
     LOG(ERROR) << "Failed to get system salt.";
     FlushTokenSaveCallbacks(false);
     state_ = STATE_NO_TOKEN;
-    FireRefreshTokensLoaded();
+    device_oauth2_token_service_delegate_->FireRefreshTokensLoaded();
     return;
   }
 
   // If the token has been set meanwhile, write it to |local_state_|.
   if (!refresh_token_.empty()) {
     EncryptAndSaveToken();
-    FireRefreshTokensLoaded();
+    device_oauth2_token_service_delegate_->FireRefreshTokensLoaded();
     return;
   }
 
@@ -258,7 +315,7 @@ void DeviceOAuth2TokenService::DidGetSystemSalt(
     if (refresh_token_.empty()) {
       LOG(ERROR) << "Failed to decrypt refresh token.";
       state_ = STATE_NO_TOKEN;
-      FireRefreshTokensLoaded();
+      device_oauth2_token_service_delegate_->FireRefreshTokensLoaded();
       return;
     }
   }
@@ -270,8 +327,9 @@ void DeviceOAuth2TokenService::DidGetSystemSalt(
     StartValidation();
 
   // Announce the token.
-  FireRefreshTokenAvailable(GetRobotAccountId());
-  FireRefreshTokensLoaded();
+  device_oauth2_token_service_delegate_->FireRefreshTokenAvailable(
+      GetRobotAccountId());
+  device_oauth2_token_service_delegate_->FireRefreshTokensLoaded();
 }
 
 void DeviceOAuth2TokenService::CheckRobotAccountId(
@@ -369,10 +427,8 @@ void DeviceOAuth2TokenService::FlushPendingRequests(
     if (token_is_valid) {
       OAuth2TokenService::FetchOAuth2Token(
           scoped_request->request.get(),
-          scoped_request->request->GetAccountId(),
-          GetRequestContext(),
-          scoped_request->client_id,
-          scoped_request->client_secret,
+          scoped_request->request->GetAccountId(), GetRequestContextImpl(),
+          scoped_request->client_id, scoped_request->client_secret,
           scoped_request->scopes);
     } else {
       FailRequest(scoped_request->request.get(), error);