Eliminate ProfileOAuth2TokenService being a BCKS.

Eliminate ProfileOAuth2TokenService being a BCKS.

This CL introduces a level of indirection around ProfileOAuth2TokenService
(ProfileOAuth2TokenServiceWrapper) to eliminate PO2TS being a BCKS. The only
purpose of the wrapper class is to glue a PO2TS instance to a Profile; the
wrapper has no public API. In particular, PO2TSFactory still vends PO2TS
instances.

BUG=334187
TBR=jochen

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

[blundell, 2014-02-10 16:19:16]

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/androi...
File chrome/browser/signin/android_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/androi...
chrome/browser/signin/android_profile_oauth2_token_service.h:71: friend class
ProfileOAuth2TokenServiceWrapperImpl;
Alternatively, perhaps I should just make these classes' constructors public?
That seems cleaner to me as I think about it.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:15: class
FakeProfileOAuth2TokenServiceWrapper
This class will eventually have to be split into its own file as the FakePO2TS
will move into the component. Should I do that in this CL do you think?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.h:113: static
BrowserContextKeyedService* Build(content::BrowserContext* context);
These functions should move to be on the FakePO2TSWrapper. Should I do that in
this CL do you think?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service.h:24: // TODO(blundell): What
should the following be rewritten to?
Suggestions here?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
File chrome/browser/signin/profile_oauth2_token_service_factory.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service_factory.h:21: class
ProfileOAuth2TokenServiceWrapper : public BrowserContextKeyedService {
It's necessary for this to be an interface so that the FakePO2TSWrapper can
implement it without inheriting the real code.

[Roger Tawa OOO till Jul 10th, 2014-02-10 19:49:13]

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/androi...
File chrome/browser/signin/android_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/androi...
chrome/browser/signin/android_profile_oauth2_token_service.h:71: friend class
ProfileOAuth2TokenServiceWrapperImpl;
On 2014/02/10 16:19:16, blundell wrote:
> Alternatively, perhaps I should just make these classes' constructors public?
> That seems cleaner to me as I think about it.

In a non-BCKS world, like iOS, will the ctor need to be public?  If so, then
makes sense and would be cleaner.  Right now the advantage is that anyone who
wants to get a pointer to a PO2TS needs to call the factory.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:15: class
FakeProfileOAuth2TokenServiceWrapper
On 2014/02/10 16:19:16, blundell wrote:
> This class will eventually have to be split into its own file as the FakePO2TS
> will move into the component. Should I do that in this CL do you think?

Probably makes sense to do it as part of this CL, since its about adding the new
layer of indirection.  Otherwise its a TODO that we'll need schedule later.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:32: };
Crazy idea, but if you make this class a template like:

   template<ServiceClass> class PO2TSWrapper : ... { 
     ...
     ServiceClass service_;
   };

and move the template to profile_oauth2_token_service_factory.h, you should be
able to reuse it for the fake, mutable and android cases without change.  Here
it would:

   PO2TSWrapper<FakeProfileOAuth2TokenService>

and in the real factory it would be:

   PO2TSWrapper<PlatformSpecificOAuth2TokenService>

The set_auto_post_fetch_response_on_message_loop() call can be left in
BuildAutoIssuingTokenService().  Would that be better?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.h:113: static
BrowserContextKeyedService* Build(content::BrowserContext* context);
On 2014/02/10 16:19:16, blundell wrote:
> These functions should move to be on the FakePO2TSWrapper. Should I do that in
> this CL do you think?

Probably makes sense to do it as part of this CL, since its about adding the new
layer of indirection.  Otherwise its a TODO that we'll need schedule later.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service.h:24: // TODO(blundell): What
should the following be rewritten to?
On 2014/02/10 16:19:16, blundell wrote:
> Suggestions here?

Seems like you can replace "BrowserContextKeyedService" with "component" and it
still reads correctly.  Maybe add that there is a BCKS wrapper if you are
starting with a Profile*?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
File chrome/browser/signin/profile_oauth2_token_service_factory.cc (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service_factory.cc:25: virtual
ProfileOAuth2TokenService* GetProfileOAuth2TokenService() OVERRIDE;
Nit: // ProfileOAuth2TokenServiceWrapper implementation:

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service_factory.cc:83:
wrapper->GetProfileOAuth2TokenService());
Why can't this call GetForProfile() ?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
File chrome/browser/signin/profile_oauth2_token_service_factory.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service_factory.h:21: class
ProfileOAuth2TokenServiceWrapper : public BrowserContextKeyedService {
On 2014/02/10 16:19:16, blundell wrote:
> It's necessary for this to be an interface so that the FakePO2TSWrapper can
> implement it without inheriting the real code.

Makes sense.

[blundell, 2014-02-13 16:07:06]

Thanks for all the great comments!

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/androi...
File chrome/browser/signin/android_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/androi...
chrome/browser/signin/android_profile_oauth2_token_service.h:71: friend class
ProfileOAuth2TokenServiceWrapperImpl;
Well the Android one won't need to be public to meet the needs of iOS, that's
for sure :). Will leave private for now.

On 2014/02/10 19:49:13, Roger Tawa wrote:
> On 2014/02/10 16:19:16, blundell wrote:
> > Alternatively, perhaps I should just make these classes' constructors
public?
> > That seems cleaner to me as I think about it.
> 
> In a non-BCKS world, like iOS, will the ctor need to be public?  If so, then
> makes sense and would be cleaner.  Right now the advantage is that anyone who
> wants to get a pointer to a PO2TS needs to call the factory.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
File chrome/browser/signin/fake_profile_oauth2_token_service.cc (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:15: class
FakeProfileOAuth2TokenServiceWrapper
On 2014/02/10 19:49:13, Roger Tawa wrote:
> On 2014/02/10 16:19:16, blundell wrote:
> > This class will eventually have to be split into its own file as the
FakePO2TS
> > will move into the component. Should I do that in this CL do you think?
> 
> Probably makes sense to do it as part of this CL, since its about adding the
new
> layer of indirection.  Otherwise its a TODO that we'll need schedule later.

Done.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.cc:32: };
It's actually only needed for the fake. Additionally, it's not clear to me
whether the call to set auto_post_fetch_response has to be done before the call
to Initialize(). For now I didn't take this suggestion -- let me know what you
think.

On 2014/02/10 19:49:13, Roger Tawa wrote:
> Crazy idea, but if you make this class a template like:
> 
>    template<ServiceClass> class PO2TSWrapper : ... { 
>      ...
>      ServiceClass service_;
>    };
> 
> and move the template to profile_oauth2_token_service_factory.h, you should be
> able to reuse it for the fake, mutable and android cases without change.  Here
> it would:
> 
>    PO2TSWrapper<FakeProfileOAuth2TokenService>
> 
> and in the real factory it would be:
> 
>    PO2TSWrapper<PlatformSpecificOAuth2TokenService>
> 
> The set_auto_post_fetch_response_on_message_loop() call can be left in
> BuildAutoIssuingTokenService().  Would that be better?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
File chrome/browser/signin/fake_profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/fake_p...
chrome/browser/signin/fake_profile_oauth2_token_service.h:113: static
BrowserContextKeyedService* Build(content::BrowserContext* context);
On 2014/02/10 19:49:13, Roger Tawa wrote:
> On 2014/02/10 16:19:16, blundell wrote:
> > These functions should move to be on the FakePO2TSWrapper. Should I do that
in
> > this CL do you think?
> 
> Probably makes sense to do it as part of this CL, since its about adding the
new
> layer of indirection.  Otherwise its a TODO that we'll need schedule later.

Done.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
File chrome/browser/signin/profile_oauth2_token_service.h (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service.h:24: // TODO(blundell): What
should the following be rewritten to?
Changed to "class" as "component" is an overloaded term. Don't want to add the
BCKS wrapper bit as this class will eventually move the component while the
wrapper will stay in //chrome. Thanks!

On 2014/02/10 19:49:13, Roger Tawa wrote:
> On 2014/02/10 16:19:16, blundell wrote:
> > Suggestions here?
> 
> Seems like you can replace "BrowserContextKeyedService" with "component" and
it
> still reads correctly.  Maybe add that there is a BCKS wrapper if you are
> starting with a Profile*?

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
File chrome/browser/signin/profile_oauth2_token_service_factory.cc (right):

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service_factory.cc:25: virtual
ProfileOAuth2TokenService* GetProfileOAuth2TokenService() OVERRIDE;
On 2014/02/10 19:49:13, Roger Tawa wrote:
> Nit: // ProfileOAuth2TokenServiceWrapper implementation:

Done.

https://codereview.chromium.org/148513010/diff/1/chrome/browser/signin/profil...
chrome/browser/signin/profile_oauth2_token_service_factory.cc:83:
wrapper->GetProfileOAuth2TokenService());
Great idea, done.

On 2014/02/10 19:49:13, Roger Tawa wrote:
> Why can't this call GetForProfile() ?

[Roger Tawa OOO till Jul 10th, 2014-02-14 13:25:37]

lgtm

[blundell, 2014-02-14 13:26:57]

The CQ bit was checked by blundell@chromium.org

[commit-bot: I haz the power, 2014-02-14 13:27:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/148513010/180001

[blundell, 2014-02-14 16:23:28]

TBR=jochen for //chrome/browser changes outside of //chrome/browser/signin

[blundell, 2014-02-14 16:23:41]

The CQ bit was unchecked by blundell@chromium.org

[blundell, 2014-02-14 16:23:51]

The CQ bit was checked by blundell@chromium.org

[Paweł Hajdan Jr., 2014-02-14 19:57:43]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-14 20:09:38]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-14 20:11:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/blundell@chromium.org/148513010/180001

[commit-bot: I haz the power, 2014-02-14 23:58:36]

Change committed as 251444