[Sync] Switch Extension Settings DTC to use ProfilesyncComponentsFactory

[Sync] Switch Extension Settings DTC to use ProfilesyncComponentsFactory

Extension settings now has an API call to get the underlying SyncableService.
We now call that from CreateExtensionOrAppSettingsSyncComponents, instead of
passing in the syncable service directly from the DTC.

This lets us get rid of the RunWithSyncableService complexity, allows for
easier dependency injection, and means anyone can use the sync factory
to easily get the SyncableService for Extension/App Settings.

BUG=104658
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123579

[Nicolas Zea, 2012-02-16 22:08:14]

PTAL. This change is necessary to switch ExtensionSettings over to the new DTC
format (where shared change processor is the one that call
GetSyncableServiceForType(..)).

[not at google - send to devlin, 2012-02-16 22:58:44]

LG pending thread safe comments. What is the context for needing this change?

https://chromiumcodereview.appspot.com/9414020/diff/4001/chrome/browser/exten...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

https://chromiumcodereview.appspot.com/9414020/diff/4001/chrome/browser/exten...
chrome/browser/extensions/settings/settings_apitest.cc:79:
GetBackendForSync(syncable::EXTENSION_SETTINGS)));
These tests all run on a single thread with the required message loops, so the
PostTasking should be unnecessary now; just

InitSyncWithSyncableService(
    sync_processor,
    b()->p()->gES()->s_f()->GetBackendForSync(s::E_S));

and hopefully no need for the MessageLoop:: stuff. Same below.

https://chromiumcodereview.appspot.com/9414020/diff/4001/chrome/browser/exten...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

https://chromiumcodereview.appspot.com/9414020/diff/4001/chrome/browser/exten...
chrome/browser/extensions/settings/settings_frontend.cc:164: SettingsBackend*
GetBackend() const {
DCHECK(FILE thread).

https://chromiumcodereview.appspot.com/9414020/diff/4001/chrome/browser/exten...
chrome/browser/extensions/settings/settings_frontend.cc:284: sync_backends =
backends_.find(settings_namespace::SYNC);
How can this ever not find ::SYNC? The constructor populates it and I didn't
think it ever gets depopulated.

Is it so that backends_ has no chance of being mutated and thus implying thread
safety? I don't know if that follows, if it does then cool, maybe Fred will know
more about this (in any case I think the check below "if sync_backends ==
backends.end()" should be a DCHECK/CHECK).

Note that this class was designed to "live entirely" (from an interface
perspective) on the same thread, for brain-confidence of thread safety. It would
be nice for that not to change, but if it does, oh well.

https://chromiumcodereview.appspot.com/9414020/diff/4001/chrome/browser/exten...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

https://chromiumcodereview.appspot.com/9414020/diff/4001/chrome/browser/exten...
chrome/browser/extensions/settings/settings_sync_unittest.cc:215: model_type));
Same comment here as in apitest.cc. Hopefully no need for PostTask anymore?

[Nicolas Zea, 2012-02-17 20:59:11]

The context is that we use the ProfileSyncComponentsFactory to hide the logic
for getting a copy of the SyncableService, and the ProfileSyncComponents needs 
a synchronous getter for the SyncableService (although it will be called from
the appropriate thread).

More specifically, ExtensionSettings is going to be switched to using the
NewNonFrontendDTC, and will no longer be the one calling into the
ProfileSyncComponentsFactory anymore. Instead, a generic SharedChangeProcessor
that has no datatype-specific logic will be controlling the interaction with the
PSCF.

[Nicolas Zea, 2012-02-23 00:27:58]

Comments addressed (RunAllPending call is still necessary, but I got rid of the
PostTask's).

Fred?

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_apitest.cc:79:
GetBackendForSync(syncable::EXTENSION_SETTINGS)));
On 2012/02/16 22:58:44, kalman wrote:
> These tests all run on a single thread with the required message loops, so the
> PostTasking should be unnecessary now; just
> 
> InitSyncWithSyncableService(
>     sync_processor,
>     b()->p()->gES()->s_f()->GetBackendForSync(s::E_S));
> 
> and hopefully no need for the MessageLoop:: stuff. Same below.

Done.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_frontend.cc:164: SettingsBackend*
GetBackend() const {
On 2012/02/16 22:58:44, kalman wrote:
> DCHECK(FILE thread).

Done.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_frontend.cc:284: sync_backends =
backends_.find(settings_namespace::SYNC);
On 2012/02/16 22:58:44, kalman wrote:
> How can this ever not find ::SYNC? The constructor populates it and I didn't
> think it ever gets depopulated.
> 
> Is it so that backends_ has no chance of being mutated and thus implying
thread
> safety? I don't know if that follows, if it does then cool, maybe Fred will
know
> more about this (in any case I think the check below "if sync_backends ==
> backends.end()" should be a DCHECK/CHECK).
> 
> Note that this class was designed to "live entirely" (from an interface
> perspective) on the same thread, for brain-confidence of thread safety. It
would
> be nice for that not to change, but if it does, oh well.

I was just being overcautious. Added a NOTREACHED in the else case.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_sync_unittest.cc:215: model_type));
On 2012/02/16 22:58:44, kalman wrote:
> Same comment here as in apitest.cc. Hopefully no need for PostTask anymore?

Done.

[akalin, 2012-02-23 01:39:56]

sync LGTM

http://codereview.chromium.org/9414020/diff/10008/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/settings_frontend.h (right):

http://codereview.chromium.org/9414020/diff/10008/chrome/browser/extensions/s...
chrome/browser/extensions/settings/settings_frontend.h:31: //
GetBackendForSync(), which should be called on the FILE thread.
should -> must

http://codereview.chromium.org/9414020/diff/10008/chrome/browser/extensions/s...
chrome/browser/extensions/settings/settings_frontend.h:49: // Should only be
called from the FILE thread. |type| should be either
should -> must

[Nicolas Zea, 2012-02-23 01:48:02]

Done, committing once trybots go green.

[not at google - send to devlin, 2012-02-23 02:15:38]

lgtm, but please follow up on those comments.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_apitest.cc:79:
GetBackendForSync(syncable::EXTENSION_SETTINGS)));
On 2012/02/23 00:27:58, nzea wrote:
> On 2012/02/16 22:58:44, kalman wrote:
> > These tests all run on a single thread with the required message loops, so
the
> > PostTasking should be unnecessary now; just
> > 
> > InitSyncWithSyncableService(
> >     sync_processor,
> >     b()->p()->gES()->s_f()->GetBackendForSync(s::E_S));
> > 
> > and hopefully no need for the MessageLoop:: stuff. Same below.
> 
> Done.

.. so you do still need the MessageLoop stuff then? It just got moved above the
block.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_frontend.cc:284: sync_backends =
backends_.find(settings_namespace::SYNC);
On 2012/02/23 00:27:58, nzea wrote:
> On 2012/02/16 22:58:44, kalman wrote:
> > How can this ever not find ::SYNC? The constructor populates it and I didn't
> > think it ever gets depopulated.
> > 
> > Is it so that backends_ has no chance of being mutated and thus implying
> thread
> > safety? I don't know if that follows, if it does then cool, maybe Fred will
> know
> > more about this (in any case I think the check below "if sync_backends ==
> > backends.end()" should be a DCHECK/CHECK).
> > 
> > Note that this class was designed to "live entirely" (from an interface
> > perspective) on the same thread, for brain-confidence of thread safety. It
> would
> > be nice for that not to change, but if it does, oh well.
> 
> I was just being overcautious. Added a NOTREACHED in the else case.

DCHECK_NE(sync_backends, backends_.end()) would be more concise.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_sync_unittest.cc:215: model_type));
On 2012/02/23 00:27:58, nzea wrote:
> On 2012/02/16 22:58:44, kalman wrote:
> > Same comment here as in apitest.cc. Hopefully no need for PostTask anymore?
> 
> Done.

Same response as before.

[Nicolas Zea, 2012-02-24 21:38:03]

Comments addressed.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_apitest.cc:79:
GetBackendForSync(syncable::EXTENSION_SETTINGS)));
On 2012/02/23 02:15:38, kalman wrote:
> On 2012/02/23 00:27:58, nzea wrote:
> > On 2012/02/16 22:58:44, kalman wrote:
> > > These tests all run on a single thread with the required message loops, so
> the
> > > PostTasking should be unnecessary now; just
> > > 
> > > InitSyncWithSyncableService(
> > >     sync_processor,
> > >     b()->p()->gES()->s_f()->GetBackendForSync(s::E_S));
> > > 
> > > and hopefully no need for the MessageLoop:: stuff. Same below.
> > 
> > Done.
> 
> .. so you do still need the MessageLoop stuff then? It just got moved above
the
> block.

Yes, there's apparently other parts of the code that post tasks, and they need
to have run before this (was getting crashes otherwise).

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_frontend.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_frontend.cc:284: sync_backends =
backends_.find(settings_namespace::SYNC);
On 2012/02/23 02:15:38, kalman wrote:
> On 2012/02/23 00:27:58, nzea wrote:
> > On 2012/02/16 22:58:44, kalman wrote:
> > > How can this ever not find ::SYNC? The constructor populates it and I
didn't
> > > think it ever gets depopulated.
> > > 
> > > Is it so that backends_ has no chance of being mutated and thus implying
> > thread
> > > safety? I don't know if that follows, if it does then cool, maybe Fred
will
> > know
> > > more about this (in any case I think the check below "if sync_backends ==
> > > backends.end()" should be a DCHECK/CHECK).
> > > 
> > > Note that this class was designed to "live entirely" (from an interface
> > > perspective) on the same thread, for brain-confidence of thread safety. It
> > would
> > > be nice for that not to change, but if it does, oh well.
> > 
> > I was just being overcautious. Added a NOTREACHED in the else case.
> 
> DCHECK_NE(sync_backends, backends_.end()) would be more concise.

Done.

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

http://codereview.chromium.org/9414020/diff/4001/chrome/browser/extensions/se...
chrome/browser/extensions/settings/settings_sync_unittest.cc:215: model_type));
On 2012/02/23 02:15:38, kalman wrote:
> On 2012/02/23 00:27:58, nzea wrote:
> > On 2012/02/16 22:58:44, kalman wrote:
> > > Same comment here as in apitest.cc. Hopefully no need for PostTask
anymore?
> > 
> > Done.
> 
> Same response as before.
See other comment.

[commit-bot: I haz the power, 2012-02-24 21:38:17]

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

[commit-bot: I haz the power, 2012-02-24 23:13:52]

Change committed as 123579