sync: Make ProfileSyncService a ProfileKeyedService.

sync: Make ProfileSyncService a ProfileKeyedService.

Create ProfileSyncService using a factory, and transfer ownership of the components factory
to ProfileSyncService. I deliberately left GetProfileSyncService to
keep this patch focused on the actual change and more manageable in size. It now just wraps
calls to the factory, and I'll change the rest of the callers in a follow up (there are a lot).

BUG=93922
TEST=Sync still works / can be set up / starts properly after browser restart / shuts down.

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

[tim (not reviewing), 2012-01-16 21:04:56]

Please review.  As I mentioned in the description I'm planning to kill
GetProfileSyncService + HasProfileSyncService in a follow up CL.

Thanks!

[Andrew T Wilson (Slow), 2012-01-17 20:25:40]

Mostly looks good - just concerned about code not in BuildServiceInstanceFor().

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:384: virtual bool HasProfileSyncService() = 0;
Why is this no longer const? Not a huge deal since it's going away, but it seems
odd.

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:158:
ProfileSyncService(ProfileSyncComponentsFactory* factory,
Since we're taking ownership of factory now, do we need to state that in a
comment?

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_factory.cc:48:
service->factory()->RegisterDataTypes(service);
Why does this live here and not down in BuildServiceInstanceFor()? I thought
that's where this type of initialization should live. Otherwise, people who try
to get instances of PSS without going through this static function will get an
improperly initialized object?

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_factory.cc:73: //
DependsOn(SigninManagerFactory::GetInstance());
Don't forget to uncomment this when my change re-lands (in CQ now).

[Elliot Glaysher, 2012-01-17 20:56:07]

mostly lg, I basically am with Andrew and don't understand what you're doing in
the ServiceFactory.

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_factory.cc:48:
service->factory()->RegisterDataTypes(service);
If I understand this correctly, Tim is using GetServiceForProfile(..., false) to
query if the service has been created and if it hasn't been, to create it and
then call some additional initialization stuff. I don't understand why this is
necessary. BuildServiceInstanceFor() is guarenteed to run once per |profile|.

[tim (not reviewing), 2012-01-17 21:17:26]

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service_factory.cc (right):

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service_factory.cc:48:
service->factory()->RegisterDataTypes(service);
On 2012/01/17 20:25:41, Andrew T Wilson wrote:
> Why does this live here and not down in BuildServiceInstanceFor()? I thought
> that's where this type of initialization should live. Otherwise, people who
try
> to get instances of PSS without going through this static function will get an
> improperly initialized object?

I tried to explain this in the TODO, but the reason is the
NonFrontendDataTypeController doesn't have the sync service injected, so it
calls GetForProfile.  If it does this as part of BuildServiceInstanceFor, you
wind up recursing. I want to fix this by just passing the service into the
controller, but that's a large mostly unrelated change I didn't feel was
necessary to couple here, so I decided to keep the mess localized to this one
spot and do it in two steps.

[tim (not reviewing), 2012-01-17 21:26:15]

On 2012/01/17 21:17:26, timsteele wrote:
>
http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
> File chrome/browser/sync/profile_sync_service_factory.cc (right):
> 
>
http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
> chrome/browser/sync/profile_sync_service_factory.cc:48:
> service->factory()->RegisterDataTypes(service);
> On 2012/01/17 20:25:41, Andrew T Wilson wrote:
> > Why does this live here and not down in BuildServiceInstanceFor()? I thought
> > that's where this type of initialization should live. Otherwise, people who
> try
> > to get instances of PSS without going through this static function will get
an
> > improperly initialized object?
> 
> I tried to explain this in the TODO, but the reason is the
> NonFrontendDataTypeController doesn't have the sync service injected, so it
> calls GetForProfile.  If it does this as part of BuildServiceInstanceFor, you
> wind up recursing. I want to fix this by just passing the service into the
> controller, but that's a large mostly unrelated change I didn't feel was
> necessary to couple here, so I decided to keep the mess localized to this one
> spot and do it in two steps.

I suppose I can do it first and rebase.  I'll talk to Nicolas and at least try
to send that out for review today.

[Andrew T Wilson (Slow), 2012-01-17 23:33:02]

On 2012/01/17 21:17:26, timsteele wrote:
> I tried to explain this in the TODO, but the reason is the
> NonFrontendDataTypeController doesn't have the sync service injected, so it
> calls GetForProfile.  If it does this as part of BuildServiceInstanceFor, you
> wind up recursing.

OK, I think I see what you're saying. RegisterDataTypes results in a call to
fetch the PSS, so if we do this in BuildServiceInstanceFor() we'll recurse,
whereas if we do it this way, we'll just end up with a not-yet-fully-registered
instance of PSS grabbed by NonFrontendDataTypeController (which is OK). 

I'm OK with landing it with this for now, given that you're going to fix it
shortly. So LGTM. I won't complain if you decide to refactor first, though :)

[Elliot Glaysher, 2012-01-17 23:36:55]

person who actually knows what he's talking about gives the lgtm? I'll lgtm too!

[tim (not reviewing), 2012-01-17 23:37:28]

On 2012/01/17 23:33:02, Andrew T Wilson wrote:
> On 2012/01/17 21:17:26, timsteele wrote:
> > I tried to explain this in the TODO, but the reason is the
> > NonFrontendDataTypeController doesn't have the sync service injected, so it
> > calls GetForProfile.  If it does this as part of BuildServiceInstanceFor,
you
> > wind up recursing.
> 
> OK, I think I see what you're saying. RegisterDataTypes results in a call to
> fetch the PSS, so if we do this in BuildServiceInstanceFor() we'll recurse,
> whereas if we do it this way, we'll just end up with a
not-yet-fully-registered
> instance of PSS grabbed by NonFrontendDataTypeController (which is OK). 
> 
> I'm OK with landing it with this for now, given that you're going to fix it
> shortly. So LGTM. I won't complain if you decide to refactor first, though :)

Cool, thanks. fwiw that review is here: http://codereview.chromium.org/9241029/

Not sure which I'll land first, but they'll both be going in shortly :)

[tim (not reviewing), 2012-01-17 23:44:17]

Responses to other comments + fix for mac compile uploaded.

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/profiles/prof...
File chrome/browser/profiles/profile.h (right):

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/profiles/prof...
chrome/browser/profiles/profile.h:384: virtual bool HasProfileSyncService() = 0;
On 2012/01/17 20:25:41, Andrew T Wilson wrote:
> Why is this no longer const? Not a huge deal since it's going away, but it
seems
> odd.

Because it now uses GetServiceForProfile, which takes a non const* to Profile.

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_service.h (right):

http://codereview.chromium.org/9232011/diff/2001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_service.h:158:
ProfileSyncService(ProfileSyncComponentsFactory* factory,
On 2012/01/17 20:25:41, Andrew T Wilson wrote:
> Since we're taking ownership of factory now, do we need to state that in a
> comment?

Done.

[commit-bot: I haz the power, 2012-01-20 03:37:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/9232011/17001

[commit-bot: I haz the power, 2012-01-20 06:03:09]

Try job failure for 9232011-17001 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-01-20 07:07:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/9232011/17001

[commit-bot: I haz the power, 2012-01-20 09:34:21]

Try job failure for 9232011-17001 (retry) (retry) on mac_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[tim (not reviewing), 2012-01-20 20:14:48]

On 2012/01/20 09:34:21, I haz the power (commit-bot) wrote:
> Try job failure for 9232011-17001 (retry) (retry) on mac_rel for step
> "browser_tests".
> It's a second try, previously, step "browser_tests" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

Try run looks good on patch set 3, committing (mac_rel is failing for everyone).