[Profiles, Fixit] Making GAIAInfoUpdateService into a ProfileKeyedService.

[Profiles, Fixit] Making GAIAInfoUpdateService into a ProfileKeyedService.

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

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

[rpetterson, 2012-12-18 07:29:07]

sail - profiles/*
sky - chrome/ and chrome/test/base/
bauerb - browser_prefs

[Bernhard Bauer, 2012-12-18 07:37:49]

Woot, PKS refactorings! :)

https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
File chrome/browser/profiles/gaia_info_update_service_factory.h (right):

https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
chrome/browser/profiles/gaia_info_update_service_factory.h:38: virtual bool
ServiceIsNULLWhileTesting() const OVERRIDE;
Hm, I think you need to override ServiceIsCreatedWithProfile() as well to return
true, otherwise the GAIAInfoUpdateService is never actually instantiated.

[rpetterson, 2012-12-18 08:44:59]

https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
File chrome/browser/profiles/gaia_info_update_service_factory.h (right):

https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
chrome/browser/profiles/gaia_info_update_service_factory.h:38: virtual bool
ServiceIsNULLWhileTesting() const OVERRIDE;
On 2012/12/18 07:37:49, Bernhard Bauer wrote:
> Hm, I think you need to override ServiceIsCreatedWithProfile() as well to
return
> true, otherwise the GAIAInfoUpdateService is never actually instantiated.

I intentionally didn't figuring that it would get created when it was needed --
I couldn't find anywhere that it was used that couldn't create it on the fly.
Sail would probably know for sure though. Sail?

[Bernhard Bauer, 2012-12-18 14:15:30]

https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
File chrome/browser/profiles/gaia_info_update_service_factory.h (right):

https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
chrome/browser/profiles/gaia_info_update_service_factory.h:38: virtual bool
ServiceIsNULLWhileTesting() const OVERRIDE;
On 2012/12/18 08:44:59, rpetterson wrote:
> On 2012/12/18 07:37:49, Bernhard Bauer wrote:
> > Hm, I think you need to override ServiceIsCreatedWithProfile() as well to
> return
> > true, otherwise the GAIAInfoUpdateService is never actually instantiated.
> 
> I intentionally didn't figuring that it would get created when it was needed
--
> I couldn't find anywhere that it was used that couldn't create it on the fly.
> Sail would probably know for sure though. Sail?

Well, I couldn't find anymore that it was used, period ;-) I think what happens
in the old version is that it is created with the profile and then starts
updating, but it's never used by something else, so with lazy creation it
wouldn't get created at all.

[sky, 2012-12-18 16:29:14]

LGTM

[sail, 2012-12-18 18:10:23]

On 2012/12/18 14:15:30, Bernhard Bauer wrote:
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> File chrome/browser/profiles/gaia_info_update_service_factory.h (right):
> 
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> chrome/browser/profiles/gaia_info_update_service_factory.h:38: virtual bool
> ServiceIsNULLWhileTesting() const OVERRIDE;
> On 2012/12/18 08:44:59, rpetterson wrote:
> > On 2012/12/18 07:37:49, Bernhard Bauer wrote:
> > > Hm, I think you need to override ServiceIsCreatedWithProfile() as well to
> > return
> > > true, otherwise the GAIAInfoUpdateService is never actually instantiated.
> > 
> > I intentionally didn't figuring that it would get created when it was needed
> --
> > I couldn't find anywhere that it was used that couldn't create it on the
fly.
> > Sail would probably know for sure though. Sail?
> 
> Well, I couldn't find anymore that it was used, period ;-) I think what
happens
> in the old version is that it is created with the profile and then starts
> updating, but it's never used by something else, so with lazy creation it
> wouldn't get created at all.

Yea, with this change I can no longer use my Google+ photo for my profile
avatar. To test this just run Chrome with --gaia-profile-info and sign into
sync.

To fix this GAIAInfoUpdateService should be created once the profile is
initialized.

[rpetterson, 2012-12-18 21:13:19]

On 2012/12/18 18:10:23, sail wrote:
> On 2012/12/18 14:15:30, Bernhard Bauer wrote:
> >
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> > File chrome/browser/profiles/gaia_info_update_service_factory.h (right):
> > 
> >
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> > chrome/browser/profiles/gaia_info_update_service_factory.h:38: virtual bool
> > ServiceIsNULLWhileTesting() const OVERRIDE;
> > On 2012/12/18 08:44:59, rpetterson wrote:
> > > On 2012/12/18 07:37:49, Bernhard Bauer wrote:
> > > > Hm, I think you need to override ServiceIsCreatedWithProfile() as well
to
> > > return
> > > > true, otherwise the GAIAInfoUpdateService is never actually
instantiated.
> > > 
> > > I intentionally didn't figuring that it would get created when it was
needed
> > --
> > > I couldn't find anywhere that it was used that couldn't create it on the
> fly.
> > > Sail would probably know for sure though. Sail?
> > 
> > Well, I couldn't find anymore that it was used, period ;-) I think what
> happens
> > in the old version is that it is created with the profile and then starts
> > updating, but it's never used by something else, so with lazy creation it
> > wouldn't get created at all.
> 
> Yea, with this change I can no longer use my Google+ photo for my profile
> avatar. To test this just run Chrome with --gaia-profile-info and sign into
> sync.
> 
> To fix this GAIAInfoUpdateService should be created once the profile is
> initialized.

Unfortuantely just overriding ServiceIsCreatedWithProfile won't do it. That
function is called before DoFinalInit and the prefs we need aren't created until
the start of DoFinalInit. I've uploaded a fix based on the way the code was set
up before that works (just calling GetForProfile after the prefs are in there).
Sail and I discussed this offline.

Also ccing jwd who is working on related stuff.

[Bernhard Bauer, 2012-12-18 21:31:27]

On 2012/12/18 21:13:19, rpetterson wrote:
> On 2012/12/18 18:10:23, sail wrote:
> > On 2012/12/18 14:15:30, Bernhard Bauer wrote:
> > >
> >
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> > > File chrome/browser/profiles/gaia_info_update_service_factory.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> > > chrome/browser/profiles/gaia_info_update_service_factory.h:38: virtual
bool
> > > ServiceIsNULLWhileTesting() const OVERRIDE;
> > > On 2012/12/18 08:44:59, rpetterson wrote:
> > > > On 2012/12/18 07:37:49, Bernhard Bauer wrote:
> > > > > Hm, I think you need to override ServiceIsCreatedWithProfile() as well
> to
> > > > return
> > > > > true, otherwise the GAIAInfoUpdateService is never actually
> instantiated.
> > > > 
> > > > I intentionally didn't figuring that it would get created when it was
> needed
> > > --
> > > > I couldn't find anywhere that it was used that couldn't create it on the
> > fly.
> > > > Sail would probably know for sure though. Sail?
> > > 
> > > Well, I couldn't find anymore that it was used, period ;-) I think what
> > happens
> > > in the old version is that it is created with the profile and then starts
> > > updating, but it's never used by something else, so with lazy creation it
> > > wouldn't get created at all.
> > 
> > Yea, with this change I can no longer use my Google+ photo for my profile
> > avatar. To test this just run Chrome with --gaia-profile-info and sign into
> > sync.
> > 
> > To fix this GAIAInfoUpdateService should be created once the profile is
> > initialized.
> 
> Unfortuantely just overriding ServiceIsCreatedWithProfile won't do it. That
> function is called before DoFinalInit and the prefs we need aren't created
until
> the start of DoFinalInit. I've uploaded a fix based on the way the code was
set
> up before that works (just calling GetForProfile after the prefs are in
there).
> Sail and I discussed this offline.

Hm. It seems a lot of these things could be made easier if PrefService was a PKS
as well :-/

> Also ccing jwd who is working on related stuff.

[rpetterson, 2012-12-18 22:08:14]

On 2012/12/18 21:31:27, Bernhard Bauer wrote:
> On 2012/12/18 21:13:19, rpetterson wrote:
> > On 2012/12/18 18:10:23, sail wrote:
> > > On 2012/12/18 14:15:30, Bernhard Bauer wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> > > > File chrome/browser/profiles/gaia_info_update_service_factory.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/11620012/diff/6001/chrome/browser/profiles/ga...
> > > > chrome/browser/profiles/gaia_info_update_service_factory.h:38: virtual
> bool
> > > > ServiceIsNULLWhileTesting() const OVERRIDE;
> > > > On 2012/12/18 08:44:59, rpetterson wrote:
> > > > > On 2012/12/18 07:37:49, Bernhard Bauer wrote:
> > > > > > Hm, I think you need to override ServiceIsCreatedWithProfile() as
well
> > to
> > > > > return
> > > > > > true, otherwise the GAIAInfoUpdateService is never actually
> > instantiated.
> > > > > 
> > > > > I intentionally didn't figuring that it would get created when it was
> > needed
> > > > --
> > > > > I couldn't find anywhere that it was used that couldn't create it on
the
> > > fly.
> > > > > Sail would probably know for sure though. Sail?
> > > > 
> > > > Well, I couldn't find anymore that it was used, period ;-) I think what
> > > happens
> > > > in the old version is that it is created with the profile and then
starts
> > > > updating, but it's never used by something else, so with lazy creation
it
> > > > wouldn't get created at all.
> > > 
> > > Yea, with this change I can no longer use my Google+ photo for my profile
> > > avatar. To test this just run Chrome with --gaia-profile-info and sign
into
> > > sync.
> > > 
> > > To fix this GAIAInfoUpdateService should be created once the profile is
> > > initialized.
> > 
> > Unfortuantely just overriding ServiceIsCreatedWithProfile won't do it. That
> > function is called before DoFinalInit and the prefs we need aren't created
> until
> > the start of DoFinalInit. I've uploaded a fix based on the way the code was
> set
> > up before that works (just calling GetForProfile after the prefs are in
> there).
> > Sail and I discussed this offline.
> 
> Hm. It seems a lot of these things could be made easier if PrefService was a
PKS
> as well :-/

Yes, but that's a much larger task and definitely not in the scope of this CL.

> 
> > Also ccing jwd who is working on related stuff.

[sail, 2012-12-19 23:16:00]

LGTM!
Patched this locally and verified that everything works great.

[Bernhard Bauer, 2012-12-19 23:27:15]

chrome/browser/prefs LGTM.

[commit-bot: I haz the power, 2013-01-03 00:30:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/11620012/43001

[commit-bot: I haz the power, 2013-01-03 02:41:48]

Change committed as 174923

[commit-bot: I haz the power, 2013-01-03 18:41:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlp@chromium.org/11620012/43003

[commit-bot: I haz the power, 2013-01-03 20:44:22]

Change committed as 175005