[Sync] Replace manual DI with SetTestingFactory.

[Sync] Replace manual DI with SetTestingFactory.

Previously we passed in a base::Callback to access the SyncService, but
it seems that machinery already exists within
BrowserContextKeyedServiceFactory to facilitate that.

BUG=

Review-Url: https://codereview.chromium.org/2910773003
Cr-Commit-Position: refs/heads/master@{#476042}
Committed: https://chromium.googlesource.com/chromium/src/+/1ad46b56059a2f74da00d4c42e604a2090712a73

[skym, 2017-05-26 22:44:25]

Description was changed from

==========
[Sync] Replace manual DI with SetTestingFactory.

BUG=
==========

to

==========
[Sync] Replace manual DI with SetTestingFactory.

Previously we passed in a base::Callback to access the SyncService, but
it seems that machinery already exists within
BrowserContextKeyedServiceFactory to facilitate that.

BUG=
==========

[skym, 2017-05-26 22:44:26]

skym@chromium.org changed reviewers:
+ erg@chromium.org

[skym, 2017-05-26 22:44:33]

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

[commit-bot: I haz the power, 2017-05-26 22:44:59]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skym, 2017-05-26 22:51:53]

Hey erg@, hoping to get general feedback on this approach, before sending to
rest of code owners.

1. I feel like this approach of using SetTestingFactory is better than the
previous approach of passing a base::Callback object into a class's constructor.
Curious if you disagree here.
2. It makes sense to move the KeyedService inheritance into a base class, right?
3. When unit tests only deal with a single profile, is there ever a reason to
use TestingProfile::Builder::AddTestingFactory instead of
BrowserContextKeyedServiceFactory::SetTestingFactory?

Also, I'm adding a DEP on components/keyed_service/core, which I'll need your
sign off for.

[Elliot Glaysher, 2017-05-26 23:12:06]

On 2017/05/26 22:51:53, skym wrote:
> Hey erg@, hoping to get general feedback on this approach, before sending to
> rest of code owners.
> 
> 1. I feel like this approach of using SetTestingFactory is better than the
> previous approach of passing a base::Callback object into a class's
constructor.
> Curious if you disagree here.

I agree that using SetTestingFactory is better than the callback.

> 2. It makes sense to move the KeyedService inheritance into a base class,
right?

I don't know enough about the internals of how the sync stuff is set up to have
an opinion on whether this makes sense or not.

> 3. When unit tests only deal with a single profile, is there ever a reason to
> use TestingProfile::Builder::AddTestingFactory instead of
> BrowserContextKeyedServiceFactory::SetTestingFactory?

I've never really interacted with TestingProfile::Builder, but just skimming it,
my immediate gut response is: there's not really a reason to use it in the one
profile case. Maybe if you were mocking multiple services at the same time
instead of testing one service in isolation?

(That said, I'd like to emphasize that you're asking me about code that I just
found out existed! I may be entirely wrong here!)

[commit-bot: I haz the power, 2017-05-26 23:53:36]

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

[commit-bot: I haz the power, 2017-05-26 23:53:37]

Dry run: This issue passed the CQ dry run.

[skym, 2017-05-30 15:43:55]

On 2017/05/26 23:12:06, Elliot Glaysher wrote:
> On 2017/05/26 22:51:53, skym wrote:
> > Hey erg@, hoping to get general feedback on this approach, before sending to
> > rest of code owners.
> > 
> > 1. I feel like this approach of using SetTestingFactory is better than the
> > previous approach of passing a base::Callback object into a class's
> constructor.
> > Curious if you disagree here.
> 
> I agree that using SetTestingFactory is better than the callback.
> 
> > 2. It makes sense to move the KeyedService inheritance into a base class,
> right?
> 
> I don't know enough about the internals of how the sync stuff is set up to
have
> an opinion on whether this makes sense or not.
> 
> > 3. When unit tests only deal with a single profile, is there ever a reason
to
> > use TestingProfile::Builder::AddTestingFactory instead of
> > BrowserContextKeyedServiceFactory::SetTestingFactory?
> 
> I've never really interacted with TestingProfile::Builder, but just skimming
it,
> my immediate gut response is: there's not really a reason to use it in the one
> profile case. Maybe if you were mocking multiple services at the same time
> instead of testing one service in isolation?
> 
> (That said, I'd like to emphasize that you're asking me about code that I just
> found out existed! I may be entirely wrong here!)

Thanks for the quick response! Sorry for question #2, from a sync perspective I
believe think it makes complete sense. I just wanted to make sure that
inheritance in keyed service wasn't an anti pattern or something.

[skym, 2017-05-30 15:47:46]

skym@chromium.org changed reviewers:
+ dbeam@chromium.org, pnoland@chromium.org, rkaplow@chromium.org

[skym, 2017-05-30 15:47:46]

PTAL

erg@ - keyed_service/core DEP
dbeam@ - chrome/browser/ui/webui/*
pnoland@ - sync stuff
rkaplow@ - components/ukm/*

[Elliot Glaysher, 2017-05-30 19:27:57]

deps lgtm

[Patrick Noland, 2017-05-30 23:11:56]

lgtm

[Dan Beam, 2017-05-31 00:13:06]

lgtm

[rkaplow, 2017-05-31 19:13:14]

lgtm

[skym, 2017-05-31 19:22:39]

The CQ bit was checked by skym@chromium.org

[commit-bot: I haz the power, 2017-05-31 19:24:00]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-31 21:46:54]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1496258559091770, "parent_rev":
"4e1be6ae8914f0eeed1df004b74a467bcb93241b", "commit_rev":
"1ad46b56059a2f74da00d4c42e604a2090712a73"}

[commit-bot: I haz the power, 2017-05-31 21:47:20]

Description was changed from

==========
[Sync] Replace manual DI with SetTestingFactory.

Previously we passed in a base::Callback to access the SyncService, but
it seems that machinery already exists within
BrowserContextKeyedServiceFactory to facilitate that.

BUG=
==========

to

==========
[Sync] Replace manual DI with SetTestingFactory.

Previously we passed in a base::Callback to access the SyncService, but
it seems that machinery already exists within
BrowserContextKeyedServiceFactory to facilitate that.

BUG=

Review-Url: https://codereview.chromium.org/2910773003
Cr-Commit-Position: refs/heads/master@{#476042}
Committed:
https://chromium.googlesource.com/chromium/src/+/1ad46b56059a2f74da00d4c42e60...
==========

[commit-bot: I haz the power, 2017-05-31 21:47:23]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/1ad46b56059a2f74da00d4c42e60...