sync: remove CreateSharedChangeProcessor from ProfileSyncComponentsFactory

sync: remove CreateSharedChangeProcessor from ProfileSyncComponentsFactory

This is a step towards changing SCP's dependency on ProfileSyncComponentsFactory to SyncApiComponentsFactory. This method was used exclusively for testing unlike other S.A.C.F methods which serve a production code purpose. Moreover, I found that most tests didn't actually need a SharedChangeProcessorMock yet its presence (especially when considered with CreateGenericChangeProcessor) made things more complicated, so I removed it from everywhere except NonUIDataTypeControllerTest.

BUG=339726
R=haitaol@chromium.org, maniscalco@chromium.org

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

[tim (not reviewing), 2014-04-22 17:52:03]

Haitao, please review. 

Nick, FYI.

[maniscalco, 2014-04-23 16:06:12]

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/autofill_data_type_controller.cc:33: new
SharedChangeProcessorFactory(),
nit: you can drop the parentheses (this applies elsewhere; s/new
SharedChangeProcessorFactory()/ should find them all).

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:39: using
testing::NiceMock;
Is this still needed?

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.h (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.h:41:
SharedChangeProcessorFactory* change_processor_factory,
I see that this class holds a SCPF by scoped_ptr.  Is there a reason the ctor
arg is a dumb pointer instead of scoped_ptr?  What do you think about changing
it to scoped_ptr so it's clear who owns it?

[haitaol1, 2014-04-23 20:51:39]

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (left):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:217:
EXPECT_CALL(*change_processor_.get(), Connect(_, _, _, _, _))
Should this be replaced with expected call on GetSyncableServiceForType(_)?

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:248:
EXPECT_CALL(*change_processor_.get(), Connect(_, _, _, _, _))
same here

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.cc:34:
SharedChangeProcessorFactory* change_processor_factory,
Why don't pass SharedChangeProcessor directly?

[haitaol1, 2014-04-23 21:50:35]

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.cc:34:
SharedChangeProcessorFactory* change_processor_factory,
On 2014/04/23 20:51:40, haitaol1 wrote:
> Why don't pass SharedChangeProcessor directly?

I see the reason, a new processor is created when non_ui_dtc is restarted. Seems
this factory exists only for testing purpose. Is it possible to just use
SharedChangeProcessor and mock ProfileSyncFactory in tests like
ui_data_type_controller_unittest.cc does.

[tim (not reviewing), 2014-04-25 03:49:02]

Addressed comments.

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/autofill_data_type_controller.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/autofill_data_type_controller.cc:33: new
SharedChangeProcessorFactory(),
On 2014/04/23 16:06:12, maniscalco wrote:
> nit: you can drop the parentheses (this applies elsewhere; s/new
> SharedChangeProcessorFactory()/ should find them all).

true although we generally keep them in chrome code... I think?

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (left):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:217:
EXPECT_CALL(*change_processor_.get(), Connect(_, _, _, _, _))
On 2014/04/23 20:51:40, haitaol1 wrote:
> Should this be replaced with expected call on GetSyncableServiceForType(_)?

I would say no.  In general the less gmock expect_calls for internal details the
better; it makes the tests sensitive to change but also, in this case, is
targeting the wrong thing... it isn't even up to AutofillDataTypeController to
call Connect, that's handled by the NonUIDataTypeController.

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/autofill_data_type_controller_unittest.cc:39: using
testing::NiceMock;
On 2014/04/23 16:06:12, maniscalco wrote:
> Is this still needed?

Nope, done!

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.cc:34:
SharedChangeProcessorFactory* change_processor_factory,
On 2014/04/23 21:50:35, haitaol1 wrote:
> On 2014/04/23 20:51:40, haitaol1 wrote:
> > Why don't pass SharedChangeProcessor directly?
> 
> I see the reason, a new processor is created when non_ui_dtc is restarted.
Seems
> this factory exists only for testing purpose. Is it possible to just use
> SharedChangeProcessor and mock ProfileSyncFactory in tests like
> ui_data_type_controller_unittest.cc does.
> 

No, the difference is that NonUIDataTypeControllerTest wants to do tricky things
at the SCP layer to test different code paths in the DTC.

I ran into this because I was trying to fully remove the need for a factory and
just have the DTC 'new' a SharedChangeProcessor on-site.  The method could exist
on ProfileSyncComponentsFactory but it would only be out of convenience and
leads to wider, undesired use like the other files in this patch. Any test that
uses a ProfileSyncComponentsFactory will be either A) tempted to mock out this
low level thing that they aren't concerned with or b) write an EXPECT_CALL to
return the real one.  So to scope that complexity only to the class that needs
it, plus the fact that I'm trying to shrink dependencies of the {UI, NonUI}DTC
classes to be sync/api only, I created a separate factory for
NonUIDataTypeController.

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.h (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.h:41:
SharedChangeProcessorFactory* change_processor_factory,
On 2014/04/23 16:06:12, maniscalco wrote:
> I see that this class holds a SCPF by scoped_ptr.  Is there a reason the ctor
> arg is a dumb pointer instead of scoped_ptr?  What do you think about changing
> it to scoped_ptr so it's clear who owns it?

Done.

[haitaol1, 2014-04-25 17:22:56]

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.cc:34:
SharedChangeProcessorFactory* change_processor_factory,
I think maybe it's simpler to do this:
* Add a protected method CreateProcessor(), by default it creates SCP.
* Fake DTC in unit test can override CreateProcessor to return pre-created one.


On 2014/04/25 03:49:02, timsteele wrote:
> On 2014/04/23 21:50:35, haitaol1 wrote:
> > On 2014/04/23 20:51:40, haitaol1 wrote:
> > > Why don't pass SharedChangeProcessor directly?
> > 
> > I see the reason, a new processor is created when non_ui_dtc is restarted.
> Seems
> > this factory exists only for testing purpose. Is it possible to just use
> > SharedChangeProcessor and mock ProfileSyncFactory in tests like
> > ui_data_type_controller_unittest.cc does.
> > 
> 
> No, the difference is that NonUIDataTypeControllerTest wants to do tricky
things
> at the SCP layer to test different code paths in the DTC.
> 
> I ran into this because I was trying to fully remove the need for a factory
and
> just have the DTC 'new' a SharedChangeProcessor on-site.  The method could
exist
> on ProfileSyncComponentsFactory but it would only be out of convenience and
> leads to wider, undesired use like the other files in this patch. Any test
that
> uses a ProfileSyncComponentsFactory will be either A) tempted to mock out this
> low level thing that they aren't concerned with or b) write an EXPECT_CALL to
> return the real one.  So to scope that complexity only to the class that needs
> it, plus the fact that I'm trying to shrink dependencies of the {UI, NonUI}DTC
> classes to be sync/api only, I created a separate factory for
> NonUIDataTypeController.

[tim (not reviewing), 2014-04-25 17:54:56]

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/non_ui_data_type_controller.cc (right):

https://codereview.chromium.org/247433004/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/non_ui_data_type_controller.cc:34:
SharedChangeProcessorFactory* change_processor_factory,
You're right.  Normally I prefer injecting to overriding like this, but in this
case since NonUIDataTypeControllerFake already exists it's probably better to be
consistent than add a different customization mechanism.  And it does further
isolate the other tests from the override.  

Done, ptal.

On 2014/04/25 17:22:57, haitaol1 wrote:
> I think maybe it's simpler to do this:
> * Add a protected method CreateProcessor(), by default it creates SCP.
> * Fake DTC in unit test can override CreateProcessor to return pre-created
one.
> 
> 
> On 2014/04/25 03:49:02, timsteele wrote:
> > On 2014/04/23 21:50:35, haitaol1 wrote:
> > > On 2014/04/23 20:51:40, haitaol1 wrote:
> > > > Why don't pass SharedChangeProcessor directly?
> > > 
> > > I see the reason, a new processor is created when non_ui_dtc is restarted.
> > Seems
> > > this factory exists only for testing purpose. Is it possible to just use
> > > SharedChangeProcessor and mock ProfileSyncFactory in tests like
> > > ui_data_type_controller_unittest.cc does.
> > > 
> > 
> > No, the difference is that NonUIDataTypeControllerTest wants to do tricky
> things
> > at the SCP layer to test different code paths in the DTC.
> > 
> > I ran into this because I was trying to fully remove the need for a factory
> and
> > just have the DTC 'new' a SharedChangeProcessor on-site.  The method could
> exist
> > on ProfileSyncComponentsFactory but it would only be out of convenience and
> > leads to wider, undesired use like the other files in this patch. Any test
> that
> > uses a ProfileSyncComponentsFactory will be either A) tempted to mock out
this
> > low level thing that they aren't concerned with or b) write an EXPECT_CALL
to
> > return the real one.  So to scope that complexity only to the class that
needs
> > it, plus the fact that I'm trying to shrink dependencies of the {UI,
NonUI}DTC
> > classes to be sync/api only, I created a separate factory for
> > NonUIDataTypeController.
>

[haitaol1, 2014-04-25 18:51:18]

lgtm

[tim (not reviewing), 2014-04-25 19:02:00]

The CQ bit was checked by tim@chromium.org

[commit-bot: I haz the power, 2014-04-25 22:11:07]

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

[commit-bot: I haz the power, 2014-04-25 23:45:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 23:45:42]

Try jobs failed on following builders:
  tryserver.chromium on win_chromium_rel

[maniscalco, 2014-04-28 15:53:33]

On 2014/04/25 23:45:42, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   tryserver.chromium on win_chromium_rel

lgtm

[tim (not reviewing), 2014-04-28 17:41:31]

Committed patchset #3 manually as r266597 (presubmit successful).