Remove Profile dependency from some ProfileSyncService unittests

Remove Profile dependency from some ProfileSyncService unittests

More precisely, this removes the //chrome dependencies from TestProfileSyncService, AbstractProfileSyncServiceTest, ProfileSyncServiceTypedUrlTest and ProfileSyncServiceAutofillTest.

BUG=581640
Committed: https://crrev.com/9352dd94f89103fba52d018f24d657e7884fda5d
Cr-Commit-Position: refs/heads/master@{#376446}

[vabr (Chromium), 2016-02-10 17:01:18]

Description was changed from

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false

BUG=581640
==========

to

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODOs:
     * Make chrome/browser/sync/profile_sync_service_autofill_unittest.cc work
       - FakeSyncClient::CreateModelWorkerForGroup must return some workers;
have
         a look at ChromeSyncClient::CreateModelWorkerForGroup and add some
option
         in the client builder
     * Ditto for chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
     * Fix the broken
chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc


BUG=581640
==========

[vabr (Chromium), 2016-02-11 18:49:56]

Description was changed from

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODOs:
     * Make chrome/browser/sync/profile_sync_service_autofill_unittest.cc work
       - FakeSyncClient::CreateModelWorkerForGroup must return some workers;
have
         a look at ChromeSyncClient::CreateModelWorkerForGroup and add some
option
         in the client builder
     * Ditto for chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
     * Fix the broken
chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc


BUG=581640
==========

to

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODOs:
     * Make chrome/browser/sync/profile_sync_service_typed_url_unittest.cc work:
       - DCHECK failing in syncer::BaseTransaction::BaseTransaction, apparently
a bad UserShare is getting in
     * Fix the broken
chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc


BUG=581640
==========

[vabr (Chromium), 2016-02-12 14:39:59]

Description was changed from

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODOs:
     * Make chrome/browser/sync/profile_sync_service_typed_url_unittest.cc work:
       - DCHECK failing in syncer::BaseTransaction::BaseTransaction, apparently
a bad UserShare is getting in
     * Fix the broken
chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc


BUG=581640
==========

to

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODOs:
     * Make chrome/browser/sync/profile_sync_service_typed_url_unittest.cc work.
     * Fix the broken
chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc


BUG=581640
==========

[vabr (Chromium), 2016-02-12 15:12:33]

Description was changed from

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODOs:
     * Make chrome/browser/sync/profile_sync_service_typed_url_unittest.cc work.
     * Fix the broken
chrome/browser/ui/sync/one_click_signin_sync_observer_unittest.cc


BUG=581640
==========

to

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODO -- just clean this up

BUG=581640
==========

[vabr (Chromium), 2016-02-12 16:50:48]

Description was changed from

==========
[WIP] Remove Profile dependency from TestProfileSyncService

This is WIP, not ready for review yet, not finished.
COMMIT=false
TODO -- just clean this up

BUG=581640
==========

to

==========
Remove Profile dependency from some ProfileSyncService unittests

More precisely, this removes the //chrome dependencies from
TestProfileSyncService, AbstractProfileSyncServiceTest,
ProfileSyncServiceTypedUrlTest and ProfileSyncServiceAutofillTest.

BUG=581640
==========

[vabr (Chromium), 2016-02-12 17:44:37]

vabr@chromium.org changed reviewers:
+ sdefresne@chromium.org, zea@chromium.org

[vabr (Chromium), 2016-02-12 17:44:38]

Dear reviewers!

Please have a look:

sdefresne@: components/history/core/browser/history_service.h

zea@: the rest

Thank you!
Vaclav

[Nicolas Zea, 2016-02-13 01:24:22]

Mostly LG, some nits

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
File chrome/browser/sync/abstract_profile_sync_service_test.h (right):

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:53: // Creates a
TestProfileSyncService instance based on
nit: move this method above the thread_bundle_ member variable

In fact, it would be great if we didn't have protected member variables (and
instead had getters/setters as need be), although making that change might be
too involved for this patch. Up to you.

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:56: //
SyncManagerForProfileSyncTest to be used byNotifyInitializationSuccess.
nit: "byNotifyInitializationSuccess"

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:59: base::Closure
callback,
maybe call this initialization_success_callback to make it clearer? Also I think
swapping the order of these params would be better, as in a sense the callback
is the output, the sync_client is the input.

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
File components/browser_sync/browser/profile_sync_test_util.cc (right):

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.cc:225: ~ThreadProvider()
= default;
nit: remove newline above

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
File components/browser_sync/browser/profile_sync_test_util.h (right):

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.h:100: // The client will
call this callback to produce the service.
Would be good to differentiate the comment for this method and the one above. If
anything specify that one is for the datatype specific SyncableService, the
other is the profile specific SyncService.

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.h:162:
scoped_refptr<base::SingleThreadTaskRunner> db_thread() { return db_thread_; }
nit: newline above

[vabr (Chromium), 2016-02-13 21:44:04]

Thanks, Nicolas,
For the review. I addressed your comments, PTAL.

Cheers,
Vaclav

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
File chrome/browser/sync/abstract_profile_sync_service_test.h (right):

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:53: // Creates a
TestProfileSyncService instance based on
On 2016/02/13 01:24:22, Nicolas Zea wrote:
> nit: move this method above the thread_bundle_ member variable

Done.

> 
> In fact, it would be great if we didn't have protected member variables (and
> instead had getters/setters as need be), although making that change might be
> too involved for this patch. Up to you.

I would prefer to do it in a separate CL to keep this focussed.

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:56: //
SyncManagerForProfileSyncTest to be used byNotifyInitializationSuccess.
On 2016/02/13 01:24:22, Nicolas Zea wrote:
> nit: "byNotifyInitializationSuccess"

Done.

https://codereview.chromium.org/1646553002/diff/160001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:59: base::Closure
callback,
On 2016/02/13 01:24:21, Nicolas Zea wrote:
> maybe call this initialization_success_callback to make it clearer? Also I
think
> swapping the order of these params would be better, as in a sense the callback
> is the output, the sync_client is the input.

Done.

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
File components/browser_sync/browser/profile_sync_test_util.cc (right):

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.cc:225: ~ThreadProvider()
= default;
On 2016/02/13 01:24:22, Nicolas Zea wrote:
> nit: remove newline above

Done.

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
File components/browser_sync/browser/profile_sync_test_util.h (right):

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.h:100: // The client will
call this callback to produce the service.
On 2016/02/13 01:24:22, Nicolas Zea wrote:
> Would be good to differentiate the comment for this method and the one above.
If
> anything specify that one is for the datatype specific SyncableService, the
> other is the profile specific SyncService.

Done, sorry for the copy-pasta!

https://codereview.chromium.org/1646553002/diff/160001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.h:162:
scoped_refptr<base::SingleThreadTaskRunner> db_thread() { return db_thread_; }
On 2016/02/13 01:24:22, Nicolas Zea wrote:
> nit: newline above

Done.

[vabr (Chromium), 2016-02-13 22:41:58]

Also, I realised that I have been destroying the WebDataServiceWrapper too soon,
so fixed that in patch set 11.

Cheers,
Vaclav

[sdefresne, 2016-02-15 09:47:12]

components/history lgtm

[vabr (Chromium), 2016-02-16 16:43:11]

Thanks, sdefresne@.

zea@ -- this CL is also ready for review, I addressed your past comments, and
fixed one object lifetime issue.

Cheers,
Vaclav

[Nicolas Zea, 2016-02-16 22:52:40]

Looks like you didn't diff off the other patchset?

https://codereview.chromium.org/1646553002/diff/200001/chrome/browser/sync/ab...
File chrome/browser/sync/abstract_profile_sync_service_test.h (right):

https://codereview.chromium.org/1646553002/diff/200001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:59: base::Closure
initialization_success_callback);
nit: pass callback as const ref?

[vabr (Chromium), 2016-02-17 13:53:38]

Thanks, Nicolas.

I addressed your comment. Also, I had to cherry pick a fix for shutdown-order
failure from https://codereview.chromium.org/1684643002/ into the Typed URL test
(did not surface with this CL before, because it is flaky in nature).

> Looks like you didn't diff off the other patchset?
I'm not sure what happened. Rietveld's "depends on patchset" link looked
correct, but the base files did not. I now spot-checked the CLs and they look as
expected.

Cheers,
Vaclav

https://codereview.chromium.org/1646553002/diff/200001/chrome/browser/sync/ab...
File chrome/browser/sync/abstract_profile_sync_service_test.h (right):

https://codereview.chromium.org/1646553002/diff/200001/chrome/browser/sync/ab...
chrome/browser/sync/abstract_profile_sync_service_test.h:59: base::Closure
initialization_success_callback);
On 2016/02/16 22:52:40, Nicolas Zea wrote:
> nit: pass callback as const ref?

Done.

[Nicolas Zea, 2016-02-17 22:19:06]

Looks like several of the areas I had comments from the other patch have been
pulled into this patch?

https://codereview.chromium.org/1646553002/diff/280001/components/browser_syn...
File components/browser_sync/browser/profile_sync_test_util.h (right):

https://codereview.chromium.org/1646553002/diff/280001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.h:69:
ProfileSyncServiceBundle(
I just realized my mental model for the patchsets was backwards. The other
patchset depends on this one. The style nits I had there apply here too, namely
prefer const scoped_refptr<..>& (same with base::Callback/base::Closure).

[vabr (Chromium), 2016-02-18 13:04:09]

Thanks, Nicolas!

> Looks like several of the areas I had comments from the
> other patch have been pulled into this patch?

Yes, that's what I meant when I wrote about cherry-picking in my last e-mail.

In general, sorry about the order of the CLs being confusing.

This one is the first one, https://codereview.chromium.org/1684643002/ is the
//content-removal follow-up.

I addressed all your comments across these two CLs, PTAL.

Cheers,
Vaclav

https://codereview.chromium.org/1646553002/diff/280001/components/browser_syn...
File components/browser_sync/browser/profile_sync_test_util.h (right):

https://codereview.chromium.org/1646553002/diff/280001/components/browser_syn...
components/browser_sync/browser/profile_sync_test_util.h:69:
ProfileSyncServiceBundle(
On 2016/02/17 22:19:06, Nicolas Zea wrote:
> I just realized my mental model for the patchsets was backwards. The other
> patchset depends on this one. The style nits I had there apply here too,
namely
> prefer const scoped_refptr<..>& (same with base::Callback/base::Closure).

Sorry for the chain of CLs being confusing.
I am walking through both CLs looking for the scoped_refptrs and callbacks and
applying your guidance.

[Nicolas Zea, 2016-02-18 19:11:11]

LGTM with a nit. Thanks again!

https://codereview.chromium.org/1646553002/diff/300001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

https://codereview.chromium.org/1646553002/diff/300001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:174:
base::WaitableEvent history_service_created(false, false);
optional nit: could use the RunLoop + PostTaskAndReply pattern with the
CreateHistoryService and DeleteSyncableService methods too, just to be
consistent (it's also I find a cleaner pattern since you're sure all the work in
the method running on the other thread is complete, rather than just everything
up to the signal event).

[vabr (Chromium), 2016-02-19 09:09:01]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 09:10:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1646553002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1646553002/340001

[vabr (Chromium), 2016-02-19 09:11:04]

Thanks, Nicolas,

I really appreciated all your reviews.

I'm sorry for the pushbacks and back-and-forth, but it helped me better
understand what I did here, so thanks for your patience there.

I will now be landing the whole series, hopefully all will go smoothly.

Cheers,
Vaclav

https://codereview.chromium.org/1646553002/diff/300001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service_typed_url_unittest.cc (right):

https://codereview.chromium.org/1646553002/diff/300001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service_typed_url_unittest.cc:174:
base::WaitableEvent history_service_created(false, false);
On 2016/02/18 19:11:11, Nicolas Zea wrote:
> optional nit: could use the RunLoop + PostTaskAndReply pattern with the
> CreateHistoryService and DeleteSyncableService methods too, just to be
> consistent (it's also I find a cleaner pattern since you're sure all the work
in
> the method running on the other thread is complete, rather than just
everything
> up to the signal event).

Great idea!
Done.

[vabr (Chromium), 2016-02-19 11:08:36]

The CQ bit was unchecked by vabr@chromium.org

[vabr (Chromium), 2016-02-19 13:06:52]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-02-19 13:07:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1646553002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1646553002/340001

[commit-bot: I haz the power, 2016-02-19 15:48:52]

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

[commit-bot: I haz the power, 2016-02-19 15:48:53]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2016-02-19 15:54:29]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2016-02-19 15:54:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
sdefresne@chromium.org, zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1646553002/#ps340001
(title: "Less WaitableEvents")

[commit-bot: I haz the power, 2016-02-19 15:58:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1646553002/340001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1646553002/340001

[commit-bot: I haz the power, 2016-02-19 16:09:57]

Description was changed from

==========
Remove Profile dependency from some ProfileSyncService unittests

More precisely, this removes the //chrome dependencies from
TestProfileSyncService, AbstractProfileSyncServiceTest,
ProfileSyncServiceTypedUrlTest and ProfileSyncServiceAutofillTest.

BUG=581640
==========

to

==========
Remove Profile dependency from some ProfileSyncService unittests

More precisely, this removes the //chrome dependencies from
TestProfileSyncService, AbstractProfileSyncServiceTest,
ProfileSyncServiceTypedUrlTest and ProfileSyncServiceAutofillTest.

BUG=581640
==========

[commit-bot: I haz the power, 2016-02-19 16:09:58]

Committed patchset #18 (id:340001)

[commit-bot: I haz the power, 2016-02-19 16:11:43]

Description was changed from

==========
Remove Profile dependency from some ProfileSyncService unittests

More precisely, this removes the //chrome dependencies from
TestProfileSyncService, AbstractProfileSyncServiceTest,
ProfileSyncServiceTypedUrlTest and ProfileSyncServiceAutofillTest.

BUG=581640
==========

to

==========
Remove Profile dependency from some ProfileSyncService unittests

More precisely, this removes the //chrome dependencies from
TestProfileSyncService, AbstractProfileSyncServiceTest,
ProfileSyncServiceTypedUrlTest and ProfileSyncServiceAutofillTest.

BUG=581640

Committed: https://crrev.com/9352dd94f89103fba52d018f24d657e7884fda5d
Cr-Commit-Position: refs/heads/master@{#376446}
==========

[commit-bot: I haz the power, 2016-02-19 16:11:45]

Patchset 18 (id:??) landed as
https://crrev.com/9352dd94f89103fba52d018f24d657e7884fda5d
Cr-Commit-Position: refs/heads/master@{#376446}