Extract OAuth2AccessTokenFetcher interface.

Extract OAuth2AccessTokenFetcher interface.

This CL extracts the interface of OAuth2AccessTokenFetcher as the 
implementation for mobile is different .
It renames the existing OAuth2AccessTokenFetcher to 
OAuth2AccessTokenFetcherImpl.

TBR=davemoore@chromium.org
BUG=NONE

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=254966

[msarda, 2014-02-28 20:23:46]

A part of the code style changes in the moved files come from running "git cl
format" on the file that was moved.

[Joao da Silva, 2014-03-03 20:58:19]

policy changes lgtm

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.cc:274:
scoped_ptr<base::DictionaryValue> ParseGetAccessTokenResponse(
Seems like this should be static or in the anonymous namespace

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.h (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.h:51:
OAuth2AccessTokenFetcherImpl(net::URLRequestContextGetter* getter);
explicit

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service.h:22: #include
"google_apis/gaia/oauth2_access_token_fetcher_impl.h"
Move to .cc?

[Roger Tawa OOO till Jul 10th, 2014-03-03 21:24:44]

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.h (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.h:59:
OAuth2AccessTokenConsumer* consumer) OVERRIDE;
Why move |consumer| from ctor to here?

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service.cc:254: this);
Maybe define a virtual CreateAccessTokenFetcher() method so that you can
override for iOS without forking?

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service_unittest.cc:12: #include
"google_apis/gaia/oauth2_access_token_fetcher_impl.h"
Why this change?

[msarda, 2014-03-04 12:37:13]

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.cc:274:
scoped_ptr<base::DictionaryValue> ParseGetAccessTokenResponse(
On 2014/03/03 20:58:19, Joao da Silva wrote:
> Seems like this should be static or in the anonymous namespace

I added a TODO.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.h (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.h:51:
OAuth2AccessTokenFetcherImpl(net::URLRequestContextGetter* getter);
On 2014/03/03 20:58:19, Joao da Silva wrote:
> explicit

Done.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.h:59:
OAuth2AccessTokenConsumer* consumer) OVERRIDE;
On 2014/03/03 21:24:44, Roger Tawa wrote:
> Why move |consumer| from ctor to here?

I did this to have a sane interface for OAuth2AccessTokenFetcher. Otherwise, the
interface wouldn't have known the consumer so it would have been hard to explain
what an interface with Start(), CancelRequest() does. If you do not like this, I
can consider adding a virtual SetConsumer(OAuth2AccessTokenConsumer* consumer)
=0 method in the interface. However the OAuth2AccessTokenFetcher is only meant
to handle a single request and I think this Start method makes sense.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service.cc:254: this);
On 2014/03/03 21:24:44, Roger Tawa wrote:
> Maybe define a virtual CreateAccessTokenFetcher() method so that you can
> override for iOS without forking?

I plan to do that in a separate CL. Just to separate the concerns.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service.h (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service.h:22: #include
"google_apis/gaia/oauth2_access_token_fetcher_impl.h"
On 2014/03/03 20:58:19, Joao da Silva wrote:
> Move to .cc?

Done.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_token_service_unittest.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_token_service_unittest.cc:12: #include
"google_apis/gaia/oauth2_access_token_fetcher_impl.h"
On 2014/03/03 21:24:44, Roger Tawa wrote:
> Why this change?

Done.

[Roger Tawa OOO till Jul 10th, 2014-03-04 15:20:01]

Thanks Mihai.  A couple of comments below.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.cc:274:
scoped_ptr<base::DictionaryValue> ParseGetAccessTokenResponse(
On 2014/03/04 12:37:13, msarda wrote:
> On 2014/03/03 20:58:19, Joao da Silva wrote:
> > Seems like this should be static or in the anonymous namespace
> 
> I added a TODO.

Should just do it here.  Put the function in an anon namespace, or just mark it
static.  Its not called anywhere else.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.h (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.h:59:
OAuth2AccessTokenConsumer* consumer) OVERRIDE;
On 2014/03/04 12:37:13, msarda wrote:
> On 2014/03/03 21:24:44, Roger Tawa wrote:
> > Why move |consumer| from ctor to here?
> 
> I did this to have a sane interface for OAuth2AccessTokenFetcher. Otherwise,
the
> interface wouldn't have known the consumer so it would have been hard to
explain
> what an interface with Start(), CancelRequest() does. If you do not like this,
I
> can consider adding a virtual SetConsumer(OAuth2AccessTokenConsumer* consumer)
> =0 method in the interface. However the OAuth2AccessTokenFetcher is only meant
> to handle a single request and I think this Start method makes sense.

I think you can keep the |consumer| in the ctor of the base and derived classes.
 The base class saves the pointer and can have protected methods like
FireGetTokenSuccess(token, time) and FireGetTokenFailure(error).  The
documentation for Start() and CancelRequest() can state that derived classes
will call the consumer as appropriate.

As you say, the fetcher is not meant to be reused, so adding a SetConsumer()
seems incorrect.  And for the same reason it seems incorrect to add |consumer|
to Start().

[msarda, 2014-03-04 16:13:53]

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.cc (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.cc:274:
scoped_ptr<base::DictionaryValue> ParseGetAccessTokenResponse(
On 2014/03/04 15:20:02, Roger Tawa wrote:
> On 2014/03/04 12:37:13, msarda wrote:
> > On 2014/03/03 20:58:19, Joao da Silva wrote:
> > > Seems like this should be static or in the anonymous namespace
> > 
> > I added a TODO.
> 
> Should just do it here.  Put the function in an anon namespace, or just mark
it
> static.  Its not called anywhere else.

Done.

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
File google_apis/gaia/oauth2_access_token_fetcher_impl.h (right):

https://codereview.chromium.org/182573003/diff/90001/google_apis/gaia/oauth2_...
google_apis/gaia/oauth2_access_token_fetcher_impl.h:59:
OAuth2AccessTokenConsumer* consumer) OVERRIDE;
On 2014/03/04 15:20:02, Roger Tawa wrote:
> On 2014/03/04 12:37:13, msarda wrote:
> > On 2014/03/03 21:24:44, Roger Tawa wrote:
> > > Why move |consumer| from ctor to here?
> > 
> > I did this to have a sane interface for OAuth2AccessTokenFetcher. Otherwise,
> the
> > interface wouldn't have known the consumer so it would have been hard to
> explain
> > what an interface with Start(), CancelRequest() does. If you do not like
this,
> I
> > can consider adding a virtual SetConsumer(OAuth2AccessTokenConsumer*
consumer)
> > =0 method in the interface. However the OAuth2AccessTokenFetcher is only
meant
> > to handle a single request and I think this Start method makes sense.
> 
> I think you can keep the |consumer| in the ctor of the base and derived
classes.
>  The base class saves the pointer and can have protected methods like
> FireGetTokenSuccess(token, time) and FireGetTokenFailure(error).  The
> documentation for Start() and CancelRequest() can state that derived classes
> will call the consumer as appropriate.
> 
> As you say, the fetcher is not meant to be reused, so adding a SetConsumer()
> seems incorrect.  And for the same reason it seems incorrect to add |consumer|
> to Start().

Done.

[Roger Tawa OOO till Jul 10th, 2014-03-04 16:46:53]

lgtm

Thanks Mihai.

https://codereview.chromium.org/182573003/diff/150001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_access_token_fetcher.h (right):

https://codereview.chromium.org/182573003/diff/150001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_access_token_fetcher.h:54: OAuth2AccessTokenConsumer*
consumer_;
Can probably keep the "const" now like line 104 in old code.

[msarda, 2014-03-04 16:49:40]

The CQ bit was checked by msarda@chromium.org

[msarda, 2014-03-04 16:49:49]

https://codereview.chromium.org/182573003/diff/150001/google_apis/gaia/oauth2...
File google_apis/gaia/oauth2_access_token_fetcher.h (right):

https://codereview.chromium.org/182573003/diff/150001/google_apis/gaia/oauth2...
google_apis/gaia/oauth2_access_token_fetcher.h:54: OAuth2AccessTokenConsumer*
consumer_;
On 2014/03/04 16:46:54, Roger Tawa wrote:
> Can probably keep the "const" now like line 104 in old code.

Done.

[commit-bot: I haz the power, 2014-03-04 16:51:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/182573003/170001

[commit-bot: I haz the power, 2014-03-04 17:23:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 17:23:57]

Retried try job too often on ios_rel_device_ninja for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...

[msarda, 2014-03-04 18:17:45]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2014-03-04 18:19:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/182573003/190001

[commit-bot: I haz the power, 2014-03-04 18:41:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 18:41:58]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[msarda, 2014-03-04 19:22:08]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2014-03-04 19:25:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/182573003/190001

[commit-bot: I haz the power, 2014-03-04 23:52:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-04 23:52:43]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, gfx_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[msarda, 2014-03-05 06:56:50]

The CQ bit was checked by msarda@chromium.org

[commit-bot: I haz the power, 2014-03-05 06:57:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/msarda@chromium.org/182573003/190001

[commit-bot: I haz the power, 2014-03-05 06:59:39]

Change committed as 254966