[Sync] Remove SyncableServiceAdapter.

[Sync] Remove SyncableServiceAdapter.

This comprises the following changes:
- Convert all UI thread datatypes using SyncableService API to use the new
  UIDataTypeController
- Convert ExtensionSettings to use NewNonFrontendDataTypeController (soon to be renamed
  NonUIDataTypeController).
- Have all datatypes using the SyncableService api accessible under one
  GetSyncableServiceForType(..) method in the ProfileSyncComponentsFactory.
- Fix up a bunch of tests.

BUG=104658
TEST=


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

[Nicolas Zea, 2012-02-18 00:55:15]

PTAL! This should be of the adapter! Most of this is code removal/switching to
new interface. The main changes are adding UIDataTypeController and various
testing changes.

[Nicolas Zea, 2012-02-24 20:17:50]

ping!

[akalin, 2012-02-24 23:54:02]

initial comments

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/gen...
File chrome/browser/sync/glue/generic_ui_data_type_controller.cc (right):

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/gen...
chrome/browser/sync/glue/generic_ui_data_type_controller.cc:21: type_(type) {
any particular reason why you can't just make UIDTC take the type_ param too? 
It's a bit confusing to have two classes that are pretty similar...

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
File chrome/browser/sync/glue/ui_data_type_controller.cc (right):

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
chrome/browser/sync/glue/ui_data_type_controller.cc:90:
scoped_ptr<GenericChangeProcessor> sync_processor_(
remove trailing _

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
chrome/browser/sync/glue/ui_data_type_controller.cc:108: SyncError error
=sync_processor_->GetSyncDataForType(
space after =

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
chrome/browser/sync/glue/ui_data_type_controller.cc:115: // Takes ownership of
sync_processor.
this should be right above the MDASS call

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
chrome/browser/sync/glue/ui_data_type_controller.cc:134:
StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK);
can you invert the condition? (and the last two parts)

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
chrome/browser/sync/glue/ui_data_type_controller.cc:160: // invoking the
callback will trigger a call to STOP(), which will get
STOP() -> Stop()

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
File chrome/browser/sync/glue/ui_data_type_controller.h (right):

http://codereview.chromium.org/9395058/diff/2001/chrome/browser/sync/glue/ui_...
chrome/browser/sync/glue/ui_data_type_controller.h:15: #include
"chrome/browser/sync/api/syncable_service.h"
can you get away with forward-declaring SyncableService? (Not sure if WeakPtr
needs a definition)

[Nicolas Zea, 2012-03-01 20:50:25]

PTAL

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
File chrome/browser/sync/glue/generic_ui_data_type_controller.cc (right):

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
chrome/browser/sync/glue/generic_ui_data_type_controller.cc:21: type_(type) {
On 2012/02/24 23:54:02, akalin wrote:
> any particular reason why you can't just make UIDTC take the type_ param too? 
> It's a bit confusing to have two classes that are pretty similar...

My main concern was that by having an implementation of the UIDTC always specify
the type (because the type() method in UIDTC remains purely virtual), you ensure
there's no possibility of using the wrong DTC for a specific type. I figured
it's less ambiguous when a UIDTC isn't standalone, and you have to specify some
implementation of it.

But, you're right, it would be simpler to have UIDTC just take the type as a
param and get rid of the Generic. Not sure my concerns are that valid. WDYT?

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
File chrome/browser/sync/glue/ui_data_type_controller.cc (right):

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
chrome/browser/sync/glue/ui_data_type_controller.cc:90:
scoped_ptr<GenericChangeProcessor> sync_processor_(
On 2012/02/24 23:54:02, akalin wrote:
> remove trailing _

Done.

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
chrome/browser/sync/glue/ui_data_type_controller.cc:108: SyncError error
=sync_processor_->GetSyncDataForType(
On 2012/02/24 23:54:02, akalin wrote:
> space after =

Done.

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
chrome/browser/sync/glue/ui_data_type_controller.cc:115: // Takes ownership of
sync_processor.
On 2012/02/24 23:54:02, akalin wrote:
> this should be right above the MDASS call

Done.

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
chrome/browser/sync/glue/ui_data_type_controller.cc:134:
StartDone(!sync_has_nodes ? OK_FIRST_RUN : OK);
On 2012/02/24 23:54:02, akalin wrote:
> can you invert the condition? (and the last two parts)

Done.

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
chrome/browser/sync/glue/ui_data_type_controller.cc:160: // invoking the
callback will trigger a call to STOP(), which will get
On 2012/02/24 23:54:02, akalin wrote:
> STOP() -> Stop()

Done.

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

https://chromiumcodereview.appspot.com/9395058/diff/2001/chrome/browser/sync/...
chrome/browser/sync/glue/ui_data_type_controller.h:15: #include
"chrome/browser/sync/api/syncable_service.h"
On 2012/02/24 23:54:02, akalin wrote:
> can you get away with forward-declaring SyncableService? (Not sure if WeakPtr
> needs a definition)

Done.

[akalin, 2012-03-03 01:06:24]

> chrome/browser/sync/glue/generic_ui_data_type_controller.cc:21: type_(type) {
> On 2012/02/24 23:54:02, akalin wrote:
> > any particular reason why you can't just make UIDTC take the type_ param
too? 
> > It's a bit confusing to have two classes that are pretty similar...
> 
> My main concern was that by having an implementation of the UIDTC always
specify
> the type (because the type() method in UIDTC remains purely virtual), you
ensure
> there's no possibility of using the wrong DTC for a specific type. I figured
> it's less ambiguous when a UIDTC isn't standalone, and you have to specify
some
> implementation of it.
> 
> But, you're right, it would be simpler to have UIDTC just take the type as a
> param and get rid of the Generic. Not sure my concerns are that valid. WDYT?

I see, because type() is the only remaining abstract method.

I think if someone was gonna use UIDTC for the wrong type, they'd probably use
GenericUIDTC, too.  So it doesn't really add much for the cost of an extra
similarly-named class.  I vote get rid of it.

Although eventually I'd like to move to a world where we just have a single DTC
for UI/non-UI.  It seems to me that UI datatypes are still asynchronous (since
they need to block on other services, etc.) but they just happen to be on the UI
thread.  So we can just have one single DTC that handles async stuff, whether it
comes from being on another thread or some other reason.  But that's pretty far
away.

[akalin, 2012-03-03 01:15:15]

more comments

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service_fake.h (right):

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service_fake.h:13: class SyncableServiceFake :
public SyncableService {
i always thought it should be FakeFoo and not FooFake, but I guess we have
enough of both that it doesn't matter.

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service_fake.h:19: void Reset();
Hmm do we really need this function?

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service_fake.h:23: void
set_associate_success(bool success);
what about make these something like:

SetMergeDataAndStartSyncingError(SyncError error);
SetProcessSyncChangesError(SyncError error);

so it's obvious where the error comes from (and simplifies some code)

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ap...
File chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ap...
chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc:110:
EXPECT_CALL(*profile_sync_factory_, CreateGenericChangeProcessor(_, _, _)).
worth it to create a fake profile sync factory also?  Kinda gross that
change_processor_ leaks if this expect isn't hit

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ge...
File chrome/browser/sync/glue/generic_change_processor_fake.h (right):

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor_fake.h:24: void
set_process_success(bool success);
use SyncErrors when possible

[Nicolas Zea, 2012-03-05 21:59:30]

PTAL.

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
File chrome/browser/sync/api/syncable_service_fake.h (right):

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service_fake.h:13: class SyncableServiceFake :
public SyncableService {
On 2012/03/03 01:15:15, akalin wrote:
> i always thought it should be FakeFoo and not FooFake, but I guess we have
> enough of both that it doesn't matter.

Yeah, it does seem like that's more common. Done (this and
GenericChangeProcessor).

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service_fake.h:19: void Reset();
On 2012/03/03 01:15:15, akalin wrote:
> Hmm do we really need this function?

Had it there in case it would be useful for multiple expectations, but at this
point it's simpler to manually set the errors. Done.

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/api/syn...
chrome/browser/sync/api/syncable_service_fake.h:23: void
set_associate_success(bool success);
On 2012/03/03 01:15:15, akalin wrote:
> what about make these something like:
> 
> SetMergeDataAndStartSyncingError(SyncError error);
> SetProcessSyncChangesError(SyncError error);
> 
> so it's obvious where the error comes from (and simplifies some code)

Done.

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ap...
File chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc
(right):

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ap...
chrome/browser/sync/glue/app_notification_data_type_controller_unittest.cc:110:
EXPECT_CALL(*profile_sync_factory_, CreateGenericChangeProcessor(_, _, _)).
On 2012/03/03 01:15:15, akalin wrote:
> worth it to create a fake profile sync factory also?  Kinda gross that
> change_processor_ leaks if this expect isn't hit

Switched to using scoped ptr instead.

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ge...
File chrome/browser/sync/glue/generic_change_processor_fake.h (right):

http://codereview.chromium.org/9395058/diff/11001/chrome/browser/sync/glue/ge...
chrome/browser/sync/glue/generic_change_processor_fake.h:24: void
set_process_success(bool success);
On 2012/03/03 01:15:15, akalin wrote:
> use SyncErrors when possible

Done.

[akalin, 2012-03-06 02:33:47]

LGTM

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
File chrome/browser/sync/api/fake_syncable_service.cc (right):

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
chrome/browser/sync/api/fake_syncable_service.cc:12:
FakeSyncableService::~FakeSyncableService() {}
newline before

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
File chrome/browser/sync/api/fake_syncable_service.h (right):

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
chrome/browser/sync/api/fake_syncable_service.h:22: // Whether we're syncing or
not. Set on a successful MergeDataAndStartSyncing,
reset -> unset

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/glue/fa...
File chrome/browser/sync/glue/fake_generic_change_processor.cc (right):

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/glue/fa...
chrome/browser/sync/glue/fake_generic_change_processor.cc:19:
FakeGenericChangeProcessor::~FakeGenericChangeProcessor() {}
extra newline before

[Nicolas Zea, 2012-03-06 18:27:14]

Done, committing.

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
File chrome/browser/sync/api/fake_syncable_service.cc (right):

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
chrome/browser/sync/api/fake_syncable_service.cc:12:
FakeSyncableService::~FakeSyncableService() {}
On 2012/03/06 02:33:47, akalin wrote:
> newline before

Done.

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
File chrome/browser/sync/api/fake_syncable_service.h (right):

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/api/fak...
chrome/browser/sync/api/fake_syncable_service.h:22: // Whether we're syncing or
not. Set on a successful MergeDataAndStartSyncing,
On 2012/03/06 02:33:47, akalin wrote:
> reset -> unset

Done.

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/glue/fa...
File chrome/browser/sync/glue/fake_generic_change_processor.cc (right):

http://codereview.chromium.org/9395058/diff/22001/chrome/browser/sync/glue/fa...
chrome/browser/sync/glue/fake_generic_change_processor.cc:19:
FakeGenericChangeProcessor::~FakeGenericChangeProcessor() {}
On 2012/03/06 02:33:47, akalin wrote:
> extra newline before

Done.

[commit-bot: I haz the power, 2012-03-06 18:29:12]

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

[commit-bot: I haz the power, 2012-03-06 20:56:31]

Change committed as 125215