settings: Make DeviceOAuth2TokenServiceFactory::Get() async

chrome/browser/extensions/api/identity/identity_api.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/extensions/api/identity/identity_api.h"
 
 #include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ skipping 375 matching lines @@
 
 void IdentityGetAuthTokenFunction::OnGetTokenFailure(
     const OAuth2TokenService::Request* request,
     const GoogleServiceAuthError& error) {
   login_token_request_.reset();
   OnGaiaFlowFailure(GaiaWebAuthFlow::SERVICE_AUTH_ERROR, error, std::string());
 }
 
 #if defined(OS_CHROMEOS)
 void IdentityGetAuthTokenFunction::StartDeviceLoginAccessTokenRequest() {
-  chromeos::DeviceOAuth2TokenService* service =
-      chromeos::DeviceOAuth2TokenServiceFactory::Get();
+  chromeos::DeviceOAuth2TokenServiceFactory::Get(
+      base::Bind(&IdentityGetAuthTokenFunction::DidGetTokenService,
+                 this));

[pneubeck (no reviews), 2013/10/22 09:05:03]

should this really rely on the ref-counting and potentially extend the lifetime
of this object?

alternatively use a WeakPtr

[satorux1, 2013/10/22 09:56:43]

Inside API functions, relying on ref counting is a common practice, so I'd just
go with this, until this becomes a problem.

+}
+
+void IdentityGetAuthTokenFunction::DidGetTokenService(
+    chromeos::DeviceOAuth2TokenService* service) {

[hashimoto, 2013/10/22 08:49:51]

nit: No need to have a NULL check here?

[satorux1, 2013/10/22 09:56:43]

The original code didn't have one, but sounds like a good idea. will add it.

[Michael Courage, 2013/10/23 00:22:47]

When would the factory return NULL? Is this going to be a DCHECK or some new
error case developers might have to interpret? 

[satorux1, 2013/10/23 04:23:27]

Per the header file [1], it can return NULL during the browser startup and
shutdown. I think it can also return NULL in unit tests where
DeviceOAuth2TokenServiceFactory is not initialized.  I think it's a good
practice to check NULL.

[1]
https://codereview.chromium.org/34523003/diff/40001/chrome/browser/chromeos/s...

[satorux1, 2013/10/23 06:35:44]

added:

  if (!service) {
    CompleteFunctionWithError(identity_constants::kAuthFailure);
    return;
  }
 
I thought about doing DCHECK(service) instead as this function won't be called
during browse startup and shutdown in production, but I decided to check NULL,
as this function may be called from tests where DeviceOAuth2TokenServiceFactory
is not initialized

   // Since robot account refresh tokens are scoped down to [any-api] only,
   // request access token for [any-api] instead of login.
   OAuth2TokenService::ScopeSet scopes;
   scopes.insert(GaiaConstants::kAnyApiOAuth2Scope);
   login_token_request_ =
       service->StartRequest(service->GetRobotAccountId(),
                             scopes,
                             this);
 }
 #endif
@@ skipping 346 matching lines @@
     const IdentityAPI::TokenCacheKey& rhs) const {
   if (extension_id < rhs.extension_id)
     return true;
   else if (rhs.extension_id < extension_id)
     return false;
 
   return scopes < rhs.scopes;
 }
 
 }  // namespace extensions