[Sync] Have SyncableService's take ownership of their SyncChangeProcessor.

[Sync] Have SyncableService's take ownership of their SyncChangeProcessor.

The UIDataTypeController now uses a SharedChangeProcessor, and passes a ref
to the SyncableService's. Every SyncableService now owns its own change
processor. Additionally, we use the ScopedPtr::Pass semantics for passing
around SyncChangeProcessors to ensure SyncableServices properly take
ownership.

This, along with all the test updates it requires (most of the patch) fixes
several leaks introduced in the previous patch to remove the Syncable Service
Adapter. SyncableServiceMock is removed as it didn't play nice with
scoped_ptr parameters (and was hardly used).

BUG=117098, 117538
TEST=unit_tests with valgrind/drmemory/heapcheck
TBR=georgy@chromium.org

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

[Nicolas Zea, 2012-03-20 19:03:40]

+Fred for sync changes. I'll get respective owners for the datatype specific
parts once the basic changes LG.

[akalin, 2012-03-21 01:23:19]

I took a pass over this.  I'm still thinking about whether making the service
own the processor is the right thing to do.  passable scoped_ptrs do make it
safer, though...

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/extensions/a...
File chrome/browser/extensions/app_notification_manager_sync_unittest.cc
(right):

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/extensions/a...
chrome/browser/extensions/app_notification_manager_sync_unittest.cc:69: }
virtual destructor

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/extensions/a...
chrome/browser/extensions/app_notification_manager_sync_unittest.cc:132: return
sync_processor_delegate_.PassAs<SyncChangeProcessor>();
Perhaps instead of creating a delegate above, you could instead only create the
TestChangeProcessor when this is called and keep a (non-scoped) pointer to it? 
With comments saying that it's owned by model() and lives as long as it does,
etc.

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/extensions/s...
chrome/browser/extensions/settings/settings_apitest.cc:45: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
hmm...it seems like we either need to avoid creating the delegate (like in my
app notification comments) or decomp this into its own file or something

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service_mock.h (left):

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service_mock.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
hmm, why is this a 'M' and not a 'D'?  strange.

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/sync/glue/ui...
File chrome/browser/sync/glue/ui_data_type_controller.h (right):

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/sync/glue/ui...
chrome/browser/sync/glue/ui_data_type_controller.h:100: // when Stop() or
StartFailed() are called, and |local_service_| releases it's
are -> is
it's -> its

http://codereview.chromium.org/9749012/diff/12001/chrome/browser/sync/glue/ui...
chrome/browser/sync/glue/ui_data_type_controller.h:106: // a SyncableService
takes ownership of it's SyncChangeProcessor when
it's -> its

[Nicolas Zea, 2012-03-21 17:38:27]

PTAL.

Regarding ownership: From the point of view of non-ui datatypes, given that we
don't want to block at shutdown, having them own their processor allows us to
disconnect them and leave them on their own while their shutdown task executes
(given that they may continue to try pushing sync data before that task runs).

I think this should be the standard form, both for the sake of keeping the API
consistent (this brings us one step closer to merging the two base datatype
controllers), and because I think it's conceptually cleaner. Everything that a
SyncableService receives it owns and can do with as it likes. We just enforce
that when we stop the datatype the SyncChangeProcessor implementation stops
acting on ProcessSyncChanges calls.

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
File chrome/browser/extensions/app_notification_manager_sync_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
chrome/browser/extensions/app_notification_manager_sync_unittest.cc:69: }
On 2012/03/21 01:23:19, akalin wrote:
> virtual destructor

Done.

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
chrome/browser/extensions/app_notification_manager_sync_unittest.cc:132: return
sync_processor_delegate_.PassAs<SyncChangeProcessor>();
On 2012/03/21 01:23:19, akalin wrote:
> Perhaps instead of creating a delegate above, you could instead only create
the
> TestChangeProcessor when this is called and keep a (non-scoped) pointer to it?

> With comments saying that it's owned by model() and lives as long as it does,
> etc.

I prefer this approach as it lets you verify the behavior after a StopSyncing
call as well. If I create the TestChangeProcessor on demand, it will get
destroyed at StopSyncing time, so we're more limited.

Additionally, this approach lets you have two delegates to the same processor
(which some tests use to verify two syncable services match). Eventually I plan
to pull the SyncChangeProcessorDelegate and a FakeSyncChangeProcessor into their
own class and have all tests use those.

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
chrome/browser/extensions/settings/settings_apitest.cc:45: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
On 2012/03/21 01:23:19, akalin wrote:
> hmm...it seems like we either need to avoid creating the delegate (like in my
> app notification comments) or decomp this into its own file or something

Yeah, I plan to pull this and some form of the NoopSyncChangeProcessor above
into their own classes as part of a follow-up CL.

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
File chrome/browser/sync/api/syncable_service_mock.h (left):

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
chrome/browser/sync/api/syncable_service_mock.h:1: // Copyright (c) 2011 The
Chromium Authors. All rights reserved.
On 2012/03/21 01:23:19, akalin wrote:
> hmm, why is this a 'M' and not a 'D'?  strange.

Not sure, but it's definitely deleted in my local branch.

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
File chrome/browser/sync/glue/ui_data_type_controller.h (right):

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
chrome/browser/sync/glue/ui_data_type_controller.h:100: // when Stop() or
StartFailed() are called, and |local_service_| releases it's
On 2012/03/21 01:23:19, akalin wrote:
> are -> is
> it's -> its

Done.

https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
chrome/browser/sync/glue/ui_data_type_controller.h:106: // a SyncableService
takes ownership of it's SyncChangeProcessor when
On 2012/03/21 01:23:19, akalin wrote:
> it's -> its

Done.

[akalin, 2012-03-22 08:05:56]

I'll take a look at this first thing tomorrow.

On 2012/03/21 17:38:27, nzea wrote:
> PTAL.
> 
> Regarding ownership: From the point of view of non-ui datatypes, given that we
> don't want to block at shutdown, having them own their processor allows us to
> disconnect them and leave them on their own while their shutdown task executes
> (given that they may continue to try pushing sync data before that task runs).
> 
> I think this should be the standard form, both for the sake of keeping the API
> consistent (this brings us one step closer to merging the two base datatype
> controllers), and because I think it's conceptually cleaner. Everything that a
> SyncableService receives it owns and can do with as it likes. We just enforce
> that when we stop the datatype the SyncChangeProcessor implementation stops
> acting on ProcessSyncChanges calls.
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
> File chrome/browser/extensions/app_notification_manager_sync_unittest.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
> chrome/browser/extensions/app_notification_manager_sync_unittest.cc:69: }
> On 2012/03/21 01:23:19, akalin wrote:
> > virtual destructor
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
> chrome/browser/extensions/app_notification_manager_sync_unittest.cc:132:
return
> sync_processor_delegate_.PassAs<SyncChangeProcessor>();
> On 2012/03/21 01:23:19, akalin wrote:
> > Perhaps instead of creating a delegate above, you could instead only create
> the
> > TestChangeProcessor when this is called and keep a (non-scoped) pointer to
it?
> 
> > With comments saying that it's owned by model() and lives as long as it
does,
> > etc.
> 
> I prefer this approach as it lets you verify the behavior after a StopSyncing
> call as well. If I create the TestChangeProcessor on demand, it will get
> destroyed at StopSyncing time, so we're more limited.
> 
> Additionally, this approach lets you have two delegates to the same processor
> (which some tests use to verify two syncable services match). Eventually I
plan
> to pull the SyncChangeProcessorDelegate and a FakeSyncChangeProcessor into
their
> own class and have all tests use those.
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
> File chrome/browser/extensions/settings/settings_apitest.cc (right):
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/exte...
> chrome/browser/extensions/settings/settings_apitest.cc:45: class
> SyncChangeProcessorDelegate : public SyncChangeProcessor {
> On 2012/03/21 01:23:19, akalin wrote:
> > hmm...it seems like we either need to avoid creating the delegate (like in
my
> > app notification comments) or decomp this into its own file or something
> 
> Yeah, I plan to pull this and some form of the NoopSyncChangeProcessor above
> into their own classes as part of a follow-up CL.
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
> File chrome/browser/sync/api/syncable_service_mock.h (left):
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
> chrome/browser/sync/api/syncable_service_mock.h:1: // Copyright (c) 2011 The
> Chromium Authors. All rights reserved.
> On 2012/03/21 01:23:19, akalin wrote:
> > hmm, why is this a 'M' and not a 'D'?  strange.
> 
> Not sure, but it's definitely deleted in my local branch.
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
> File chrome/browser/sync/glue/ui_data_type_controller.h (right):
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
> chrome/browser/sync/glue/ui_data_type_controller.h:100: // when Stop() or
> StartFailed() are called, and |local_service_| releases it's
> On 2012/03/21 01:23:19, akalin wrote:
> > are -> is
> > it's -> its
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/9749012/diff/12001/chrome/browser/sync...
> chrome/browser/sync/glue/ui_data_type_controller.h:106: // a SyncableService
> takes ownership of it's SyncChangeProcessor when
> On 2012/03/21 01:23:19, akalin wrote:
> > it's -> its
> 
> Done.

[akalin, 2012-03-22 20:15:08]

LGTM

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (left):

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:429: StopSyncing(PREFERENCES);
why was this deleted? (out of curiosity)

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:130: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
since you're defining this in multiple places, make sure you don't violate ODR
(i.e., these definitions should be in an anon namespace)

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/sync/glue/ui...
File chrome/browser/sync/glue/ui_data_type_controller.h (right):

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/sync/glue/ui...
chrome/browser/sync/glue/ui_data_type_controller.h:105: // SyncableService API,
whether the datatype lives on the UI thread or not (
( before 'a' on next line

[Nicolas Zea, 2012-03-22 21:22:18]

Done. Adding owner reviewers:
+Sky for /search_engines
+Asargent for /extensions
+Georgy for webdata/

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/prefs/pref_m...
File chrome/browser/prefs/pref_model_associator.cc (left):

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/prefs/pref_m...
chrome/browser/prefs/pref_model_associator.cc:429: StopSyncing(PREFERENCES);
On 2012/03/22 20:15:08, akalin wrote:
> why was this deleted? (out of curiosity)

To be consistent with the behavior of the other types (they don't do this), and
because I think the decision to StopSyncing should be the datatype manager's.

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:130: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
On 2012/03/22 20:15:08, akalin wrote:
> since you're defining this in multiple places, make sure you don't violate ODR
> (i.e., these definitions should be in an anon namespace)

Done.

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/sync/glue/ui...
File chrome/browser/sync/glue/ui_data_type_controller.h (right):

http://codereview.chromium.org/9749012/diff/21001/chrome/browser/sync/glue/ui...
chrome/browser/sync/glue/ui_data_type_controller.h:105: // SyncableService API,
whether the datatype lives on the UI thread or not (
On 2012/03/22 20:15:08, akalin wrote:
> ( before 'a' on next line

Done.

[sky, 2012-03-22 21:45:30]

LGTM

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:148: };
DISALLOW_COPY_AND_ASSIGN

[asargent_no_longer_on_chrome, 2012-03-22 22:54:11]

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_service.h (right):

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/extensions/e...
chrome/browser/extensions/extension_service.h:607:
scoped_ptr<SyncChangeProcessor> sync_processor;
Would it be worth adding a comment here reminding people that if they add new
members to the struct, they should be sure to clear them in the implementation
of Reset?

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/extensions/s...
chrome/browser/extensions/settings/settings_apitest.cc:45: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
This looks like duplicate code from the version in
app_notification_manager_sync_unittest.cc - can we put it in it's own standalone
.h/cc file somewhere so we can share the code?

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/extensions/s...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

http://codereview.chromium.org/9749012/diff/32001/chrome/browser/extensions/s...
chrome/browser/extensions/settings/settings_sync_unittest.cc:145: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
same thing here about duplicate code

[Nicolas Zea, 2012-03-22 23:05:28]

PTAL

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
File chrome/browser/extensions/extension_service.h (right):

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
chrome/browser/extensions/extension_service.h:607:
scoped_ptr<SyncChangeProcessor> sync_processor;
On 2012/03/22 22:54:11, Antony Sargent wrote:
> Would it be worth adding a comment here reminding people that if they add new
> members to the struct, they should be sure to clear them in the implementation
> of Reset?

Done.

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
chrome/browser/extensions/settings/settings_apitest.cc:45: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
On 2012/03/22 22:54:11, Antony Sargent wrote:
> This looks like duplicate code from the version in
> app_notification_manager_sync_unittest.cc - can we put it in it's own
standalone
> .h/cc file somewhere so we can share the code?

That will be arriving as part of a follow-up patch (along with pulling the
various implementations of the above NoopSyncChangeProcessor out into a single
class as well).

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_sync_unittest.cc (right):

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
chrome/browser/extensions/settings/settings_sync_unittest.cc:145: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
On 2012/03/22 22:54:11, Antony Sargent wrote:
> same thing here about duplicate code

See previous comment.

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/sear...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/sear...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:148: };
On 2012/03/22 21:45:30, sky wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

[asargent_no_longer_on_chrome, 2012-03-22 23:14:24]

chrome/browser/extensions/* LGTM

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
File chrome/browser/extensions/settings/settings_apitest.cc (right):

https://chromiumcodereview.appspot.com/9749012/diff/32001/chrome/browser/exte...
chrome/browser/extensions/settings/settings_apitest.cc:45: class
SyncChangeProcessorDelegate : public SyncChangeProcessor {
On 2012/03/22 23:05:29, nzea wrote:
> On 2012/03/22 22:54:11, Antony Sargent wrote:
> > This looks like duplicate code from the version in
> > app_notification_manager_sync_unittest.cc - can we put it in it's own
> standalone
> > .h/cc file somewhere so we can share the code?
> 
> That will be arriving as part of a follow-up patch (along with pulling the
> various implementations of the above NoopSyncChangeProcessor out into a single
> class as well).

Ok, glad to hear it.

[Nicolas Zea, 2012-03-23 18:02:37]

Moving GeorgY to TBR since the WebData changes were cosmetic. Committing once
trybots go green.