Remove duplicated code from sync related unit tests.

Remove duplicated code from sync related unit tests.

Consolidate the various implementations of SyncChangeProcessorDelegate and
fake into sync/api.  Replace many of the fake SyncChangeProcessor
implementations with a slightly more advanced FakeSyncChangeProcessor.

TBR=brettw@chromium.org
BUG=

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

[maniscalco, 2014-02-03 18:13:26]

Hi Tim, would you please review this cleanup change?

[maniscalco, 2014-02-03 23:05:50]

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1286: new
syncer::SyncChangeProcessorDelegator(model_b()));
Unfortunately, model_b() returns a TemplateURLService*, which makes it difficult
to change SyncChangeProcessorDeletator to hold a WeakPtr<> to its recipient.

[tim (not reviewing), 2014-02-06 19:26:47]

Having a single fake SCP is great, thanks!

LGTM with nits

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
File sync/api/sync_change_processor_delegator.h (right):

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
sync/api/sync_change_processor_delegator.h:12: // A delegator for use in unit
tests.
Let's put Test in the name of this class somehow to distinguish it from
production use.  Also, I think Wrapper or Proxy (WrapperForTest, ProxyForTest,
TestWrapper, TestProxy) are better names than delegator.

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
sync/api/sync_change_processor_delegator.h:19:
SyncChangeProcessorDelegator(syncer::SyncChangeProcessor* recipient);
single arg constructors should be explicit.

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
sync/api/sync_change_processor_delegator.h:30: syncer::SyncChangeProcessor*
const recipient_;
nit - wrapped_ might be a better name.

[groby-ooo-7-16, 2014-02-06 19:52:04]

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc
(right):

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:464:
custom_dictionary->MergeDataAndStartSyncing(
nit: this indentation seems wrong. I'd expect
EXPECT_FALSE(
    custom_dictionary->MergeDataAndStartSyncing(
        syncer::DICTIONARY,
        ...)
    .error()
    .IsSet());

If clang-format is happy with this, I'm happy, though :)

[maniscalco, 2014-02-06 20:05:26]

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_sync_unittest.cc
(right):

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_sync_unittest.cc:1286: new
syncer::SyncChangeProcessorDelegator(model_b()));
On 2014/02/03 23:05:50, maniscalco wrote:
> Unfortunately, model_b() returns a TemplateURLService*, which makes it
difficult
> to change SyncChangeProcessorDeletator to hold a WeakPtr<> to its recipient.

Done.

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc
(right):

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:464:
custom_dictionary->MergeDataAndStartSyncing(
On 2014/02/06 19:52:04, groby wrote:
> nit: this indentation seems wrong. I'd expect
> EXPECT_FALSE(
>     custom_dictionary->MergeDataAndStartSyncing(
>         syncer::DICTIONARY,
>         ...)
>     .error()
>     .IsSet());
> 
> If clang-format is happy with this, I'm happy, though :)

Surprised me too.  This is what I ended up with after "git cl format".

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
File sync/api/sync_change_processor_delegator.h (right):

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
sync/api/sync_change_processor_delegator.h:12: // A delegator for use in unit
tests.
On 2014/02/06 19:26:47, timsteele wrote:
> Let's put Test in the name of this class somehow to distinguish it from
> production use.  Also, I think Wrapper or Proxy (WrapperForTest, ProxyForTest,
> TestWrapper, TestProxy) are better names than delegator. 

Good idea.  Renamed to SyncChangeProcessorWrapperForTest. Done.

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
sync/api/sync_change_processor_delegator.h:19:
SyncChangeProcessorDelegator(syncer::SyncChangeProcessor* recipient);
On 2014/02/06 19:26:47, timsteele wrote:
> single arg constructors should be explicit.

Done.

https://codereview.chromium.org/151963002/diff/310001/sync/api/sync_change_pr...
sync/api/sync_change_processor_delegator.h:30: syncer::SyncChangeProcessor*
const recipient_;
On 2014/02/06 19:26:47, timsteele wrote:
> nit - wrapped_ might be a better name.

Done.

[maniscalco, 2014-02-06 21:06:25]

Hi, would you each please review for your area?

pkotwicz - c/b/themes

stevet - c/b/search_engines

jyasskin - c/b/extensions

bauerb - c/b/managed_mode and c/b/prefs

brettw - c/b/history

dewittj - c/b/notifications

groby - c/b/spellchecker

Thanks!

[Jeffrey Yasskin, 2014-02-06 21:11:49]

extensions LGTM. Thanks for killing duplicate code!

[dewittj, 2014-02-06 21:19:35]

notifications lgtm, robliao&petewil -> CC

[robliao, 2014-02-06 21:28:02]

On 2014/02/06 21:19:35, dewittj wrote:
> notifications lgtm, robliao&petewil -> CC

chrome/browser/notifications/extension_welcome_notification_unittest.cc lgtm

[Pete Williamson, 2014-02-06 21:31:29]

changes to synced notification unit tests lgtm assuming all synced notification
unit tests pass

unit_tests --gtest_filter=ChromeNotifierServiceTest.*

[SteveT, 2014-02-06 21:36:06]

lgtm for search_engines

[groby-ooo-7-16, 2014-02-06 22:03:16]

spellcheck definitely LGTM

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc
(right):

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:464:
custom_dictionary->MergeDataAndStartSyncing(
On 2014/02/06 20:05:27, maniscalco wrote:
> On 2014/02/06 19:52:04, groby wrote:
> > nit: this indentation seems wrong. I'd expect
> > EXPECT_FALSE(
> >     custom_dictionary->MergeDataAndStartSyncing(
> >         syncer::DICTIONARY,
> >         ...)
> >     .error()
> >     .IsSet());
> > 
> > If clang-format is happy with this, I'm happy, though :)
> 
> Surprised me too.  This is what I ended up with after "git cl format".

Odd. Let's leave it for this CL - no need to fight the format gods - but I've
filed http://crbug.com/341587 to address the clang issue, in case you're
interested.

[groby-ooo-7-16, 2014-02-06 22:16:55]

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
File chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc
(right):

https://codereview.chromium.org/151963002/diff/310001/chrome/browser/spellche...
chrome/browser/spellchecker/spellcheck_custom_dictionary_unittest.cc:464:
custom_dictionary->MergeDataAndStartSyncing(
On 2014/02/06 22:03:17, groby wrote:
> On 2014/02/06 20:05:27, maniscalco wrote:
> > On 2014/02/06 19:52:04, groby wrote:
> > > nit: this indentation seems wrong. I'd expect
> > > EXPECT_FALSE(
> > >     custom_dictionary->MergeDataAndStartSyncing(
> > >         syncer::DICTIONARY,
> > >         ...)
> > >     .error()
> > >     .IsSet());
> > > 
> > > If clang-format is happy with this, I'm happy, though :)
> > 
> > Surprised me too.  This is what I ended up with after "git cl format".
> 
> Odd. Let's leave it for this CL - no need to fight the format gods - but I've
> filed http://crbug.com/341587 to address the clang issue, in case you're
> interested.

FWIW - this is semi-expected behavior from clang-format. According to thakis,
this might be a heuristic the formatter applies to make the code more readable
by not keeping params and function calls on the same indentation. TMYK :)

[Bernhard Bauer, 2014-02-07 07:52:01]

managed_mode/ and prefs/ LGTM. Thanks!

[pkotwicz, 2014-02-07 15:03:51]

chrome/browser/themes LGTM

[maniscalco, 2014-02-18 18:39:35]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-02-18 18:41:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/151963002/670001

[maniscalco, 2014-02-18 19:33:17]

The CQ bit was unchecked by maniscalco@chromium.org

[maniscalco, 2014-02-18 19:50:51]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-02-18 19:51:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/151963002/890001

[maniscalco, 2014-02-18 22:46:13]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-02-18 22:47:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/151963002/1030001

[commit-bot: I haz the power, 2014-02-18 23:04:45]

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

[commit-bot: I haz the power, 2014-02-18 23:04:46]

Retried try job too often on win_rel for step(s) base_unittests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[maniscalco, 2014-02-19 00:31:32]

The CQ bit was checked by maniscalco@chromium.org

[commit-bot: I haz the power, 2014-02-19 00:33:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/maniscalco@chromium.org/151963002/1030001

[commit-bot: I haz the power, 2014-02-19 02:15:33]

Change committed as 251926