Implement CryptAuthAccessTokenFetcher.

Implement CryptAuthAccountTokenFetcher, which performs the fetching of the CryptAuth access token.

BUG=420316
Committed: https://crrev.com/408c74140a61ca3997aad508bd68f2b2c354e167
Cr-Commit-Position: refs/heads/master@{#308207}

[Tim Song, 2014-12-10 23:35:14]

Patchset #1 (id:1) has been deleted

[Tim Song, 2014-12-10 23:38:09]

tengs@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2014-12-11 02:53:22]

https://codereview.chromium.org/792193003/diff/20001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/792193003/diff/20001/components/components_te...
components/components_tests.gyp:772:
'../google_apis/google_apis.gyp:google_apis_test_support',
nit: Please alphabetize.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h
(right):

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h:8: #include
"base/callback.h"
nit: Please include callback_forward.h instead.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc
(right):

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc:11:
OAuth2TokenService::ScopeSet GetScopes() {
nit: Please add a brief docstring.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc:19: //
Inherits from OAuth2TokenService for the token caching.
I'm not following what this comment means -- do you mean "inherits from
OAuth2TokenService::Consumer"?

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc:43: //
network roundtrip for each token request.
I don't understand this comment -- where do we take advantage of the cached
token?  Is this just saying that StartRequest() might return a cached result,
rather than making a round-trip?  If so, I'd reword the comment a bit to be
clearer.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
File
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:7:
#include "base/bind.h"
nit: If you keep the scoped_ptr, please #include the appropriate header.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:20:
void SaveAccessToken(std::string* out_token, const std::string& in_token) {
nit: Please #include the header for std::string.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:34:
new CryptAuthAccountTokenFetcher(&token_service_, kAccountId));
Can this be done in the constructor, rather than in SetUp()?  If so, you
shouldn't need the scoped_ptr.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:67:
std::string result(kInvalidResult);
I think there's actually a clang warning being added for shadowed names.  Please
use a different variable name here instead.

[Tim Song, 2014-12-12 20:48:45]

https://codereview.chromium.org/792193003/diff/20001/components/components_te...
File components/components_tests.gyp (right):

https://codereview.chromium.org/792193003/diff/20001/components/components_te...
components/components_tests.gyp:772:
'../google_apis/google_apis.gyp:google_apis_test_support',
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> nit: Please alphabetize.

Done.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h
(right):

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_access_token_fetcher.h:8: #include
"base/callback.h"
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> nit: Please include callback_forward.h instead.

Done.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
File components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc
(right):

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc:11:
OAuth2TokenService::ScopeSet GetScopes() {
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> nit: Please add a brief docstring.

Done.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc:19: //
Inherits from OAuth2TokenService for the token caching.
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> I'm not following what this comment means -- do you mean "inherits from
> OAuth2TokenService::Consumer"?

Sorry, forgot to delete this comment.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher.cc:43: //
network roundtrip for each token request.
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> I don't understand this comment -- where do we take advantage of the cached
> token?  Is this just saying that StartRequest() might return a cached result,
> rather than making a round-trip?  If so, I'd reword the comment a bit to be
> clearer.

Done.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
File
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc
(right):

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:7:
#include "base/bind.h"
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> nit: If you keep the scoped_ptr, please #include the appropriate header.

Done.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:20:
void SaveAccessToken(std::string* out_token, const std::string& in_token) {
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> nit: Please #include the header for std::string.

Done.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:34:
new CryptAuthAccountTokenFetcher(&token_service_, kAccountId));
On 2014/12/11 02:53:22, Ilya Sherman wrote:
> Can this be done in the constructor, rather than in SetUp()?  If so, you
> shouldn't need the scoped_ptr.

Done. From a simulation perspective, the token service should have been properly
configured before the fetcher is created, but that's a very minor detail, and
putting it in the constructor saves a few lines.

https://codereview.chromium.org/792193003/diff/20001/components/proximity_aut...
components/proximity_auth/cryptauth/cryptauth_account_token_fetcher_unittest.cc:67:
std::string result(kInvalidResult);
On 2014/12/11 02:53:21, Ilya Sherman wrote:
> I think there's actually a clang warning being added for shadowed names. 
Please
> use a different variable name here instead.

Done. However, I kind of like scoping the same variable as it's easy to use the
wrong variable.

On a side note, I managed to crash clang somehow writing this test.

[Ilya Sherman, 2014-12-12 22:05:30]

LGTM, thanks.

[Tim Song, 2014-12-12 23:42:44]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-12-12 23:44:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/792193003/40001

[commit-bot: I haz the power, 2014-12-13 00:36:21]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2014-12-13 00:37:13]

Patchset 2 (id:??) landed as
https://crrev.com/408c74140a61ca3997aad508bd68f2b2c354e167
Cr-Commit-Position: refs/heads/master@{#308207}