[Sync] Remove the last datatype-specific deps from sync_driver.

[Sync] Remove the last datatype-specific deps from sync_driver.

The ref-counted dependencies being injected by SyncClient are now
injected directly into the DTC's via the components factory.

BUG=543199
TBR=sdefresne

Committed: https://crrev.com/be7410fa028a9f086c8f5cedd623a6d57567799d
Cr-Commit-Position: refs/heads/master@{#364465}

[maxbogue, 2015-12-07 21:21:45]

maxbogue@chromium.org changed reviewers:
+ zea@chromium.org

[maxbogue, 2015-12-07 21:21:46]

Nicolas, PTAL!

[vabr (Chromium), 2015-12-08 10:14:27]

vabr@chromium.org changed reviewers:
+ vabr@chromium.org

[vabr (Chromium), 2015-12-08 10:14:28]

Autofill and password_manager components LGTM, with multiple instances of an
optional nit, which you are free to ignore.

Cheers,
Vaclav

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_wallet_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.h:29:
const scoped_refptr<autofill::AutofillWebDataService>& web_data_service);
nit: Just autofill::AutofillWebDataService*? (See my reasoning in
components/password_manager/sync/browser/password_data_type_controller.h).

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_data_type_controller.h:30:
const scoped_refptr<autofill::AutofillWebDataService>& web_data_service);
nit: Just autofill::AutofillWebDataService*? See my reasoning in
components/password_manager/sync/browser/password_data_type_controller.h.

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
File
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h:31:
const scoped_refptr<autofill::AutofillWebDataService>& web_data_service);
nit: Just autofill::AutofillWebDataService* ?
(See my reasoning in
components/password_manager/sync/browser/password_data_type_controller.h.)

https://codereview.chromium.org/1460723004/diff/120001/components/password_ma...
File components/password_manager/sync/browser/password_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/password_ma...
components/password_manager/sync/browser/password_data_type_controller.h:34:
const scoped_refptr<password_manager::PasswordStore>& password_store);
nit: Should this be just password_manager::PasswordStore* ?
My understanding is that the (member variable) scoped_refptr is only interested
in the referenced object itself, and there changes the reference count
accordingly.

[maxbogue, 2015-12-08 19:08:05]

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
File components/autofill/core/browser/autofill_wallet_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
components/autofill/core/browser/autofill_wallet_data_type_controller.h:29:
const scoped_refptr<autofill::AutofillWebDataService>& web_data_service);
On 2015/12/08 10:14:28, vabr (Chromium) wrote:
> nit: Just autofill::AutofillWebDataService*? (See my reasoning in
> components/password_manager/sync/browser/password_data_type_controller.h).

Acknowledged.

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
File components/autofill/core/browser/webdata/autofill_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_data_type_controller.h:30:
const scoped_refptr<autofill::AutofillWebDataService>& web_data_service);
On 2015/12/08 10:14:28, vabr (Chromium) wrote:
> nit: Just autofill::AutofillWebDataService*? See my reasoning in
> components/password_manager/sync/browser/password_data_type_controller.h.

Acknowledged.

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
File
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/autofill/co...
components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h:31:
const scoped_refptr<autofill::AutofillWebDataService>& web_data_service);
On 2015/12/08 10:14:28, vabr (Chromium) wrote:
> nit: Just autofill::AutofillWebDataService* ?
> (See my reasoning in
> components/password_manager/sync/browser/password_data_type_controller.h.)

Acknowledged.

https://codereview.chromium.org/1460723004/diff/120001/components/password_ma...
File components/password_manager/sync/browser/password_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/password_ma...
components/password_manager/sync/browser/password_data_type_controller.h:34:
const scoped_refptr<password_manager::PasswordStore>& password_store);
On 2015/12/08 10:14:28, vabr (Chromium) wrote:
> nit: Should this be just password_manager::PasswordStore* ?
> My understanding is that the (member variable) scoped_refptr is only
interested
> in the referenced object itself, and there changes the reference count
> accordingly.

I can't find any style guidelines on it, but I think we prefer const & because
that way it's obvious that the pointer is ref-counted and should be stored in a
scoped_refptr.

[Nicolas Zea, 2015-12-08 21:54:03]

nice, LGTM

https://codereview.chromium.org/1460723004/diff/120001/components/browser_syn...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/browser_syn...
components/browser_sync/browser/profile_sync_components_factory_impl.h:118: //
Members that must be fetched on the UI thread but accessed on their
comment nit: This comment is outdated now right? These are being injected, not
fetched now.

[maxbogue, 2015-12-09 00:56:18]

maxbogue@chromium.org changed reviewers:
+ sdefresne@chromium.org

[maxbogue, 2015-12-09 00:56:19]

+sdefresne for ios/*, PTAL.

[vabr (Chromium), 2015-12-09 09:07:15]

https://codereview.chromium.org/1460723004/diff/120001/components/password_ma...
File components/password_manager/sync/browser/password_data_type_controller.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/password_ma...
components/password_manager/sync/browser/password_data_type_controller.h:34:
const scoped_refptr<password_manager::PasswordStore>& password_store);
On 2015/12/08 19:08:05, maxbogue wrote:
> On 2015/12/08 10:14:28, vabr (Chromium) wrote:
> > nit: Should this be just password_manager::PasswordStore* ?
> > My understanding is that the (member variable) scoped_refptr is only
> interested
> > in the referenced object itself, and there changes the reference count
> > accordingly.
> 
> I can't find any style guidelines on it, but I think we prefer const & because
> that way it's obvious that the pointer is ref-counted and should be stored in
a
> scoped_refptr.

Sounds reasonable, LGTM.

[maxbogue, 2015-12-09 18:20:58]

https://codereview.chromium.org/1460723004/diff/120001/components/browser_syn...
File components/browser_sync/browser/profile_sync_components_factory_impl.h
(right):

https://codereview.chromium.org/1460723004/diff/120001/components/browser_syn...
components/browser_sync/browser/profile_sync_components_factory_impl.h:118: //
Members that must be fetched on the UI thread but accessed on their
On 2015/12/08 21:54:03, Nicolas Zea wrote:
> comment nit: This comment is outdated now right? These are being injected, not
> fetched now.

Deleted it, thanks.

[maxbogue, 2015-12-10 18:57:51]

Description was changed from

==========
[Sync] Remove the last datatype-specific deps from sync_driver.

The ref-counted dependencies being injected by SyncClient are now
injected directly into the DTC's via the components factory.

BUG=543199
==========

to

==========
[Sync] Remove the last datatype-specific deps from sync_driver.

The ref-counted dependencies being injected by SyncClient are now
injected directly into the DTC's via the components factory.

BUG=543199
TBR=sdefresne
==========

[maxbogue, 2015-12-10 18:58:02]

The CQ bit was checked by maxbogue@chromium.org

[maxbogue, 2015-12-10 18:58:02]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org,
zea@chromium.org
Link to the patchset: https://codereview.chromium.org/1460723004/#ps140001
(title: "Delete outdated comment.")

[commit-bot: I haz the power, 2015-12-10 18:59:40]

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

[commit-bot: I haz the power, 2015-12-10 20:43:50]

Description was changed from

==========
[Sync] Remove the last datatype-specific deps from sync_driver.

The ref-counted dependencies being injected by SyncClient are now
injected directly into the DTC's via the components factory.

BUG=543199
TBR=sdefresne
==========

to

==========
[Sync] Remove the last datatype-specific deps from sync_driver.

The ref-counted dependencies being injected by SyncClient are now
injected directly into the DTC's via the components factory.

BUG=543199
TBR=sdefresne
==========

[commit-bot: I haz the power, 2015-12-10 20:43:51]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-12-10 20:45:07]

Description was changed from

==========
[Sync] Remove the last datatype-specific deps from sync_driver.

The ref-counted dependencies being injected by SyncClient are now
injected directly into the DTC's via the components factory.

BUG=543199
TBR=sdefresne
==========

to

==========
[Sync] Remove the last datatype-specific deps from sync_driver.

The ref-counted dependencies being injected by SyncClient are now
injected directly into the DTC's via the components factory.

BUG=543199
TBR=sdefresne

Committed: https://crrev.com/be7410fa028a9f086c8f5cedd623a6d57567799d
Cr-Commit-Position: refs/heads/master@{#364465}
==========

[commit-bot: I haz the power, 2015-12-10 20:45:08]

Patchset 8 (id:??) landed as
https://crrev.com/be7410fa028a9f086c8f5cedd623a6d57567799d
Cr-Commit-Position: refs/heads/master@{#364465}