[Sync] Replace ProfileSyncComponentsFactory with SyncClient

[Sync] Replace ProfileSyncComponentsFactory with SyncClient

The ProfileSyncComponentsFactoryImpl now inherits from
SyncApiComponentsFactory, and will be renamed and componentized in a followup
patch. Everywhere else that relied on ProfileSyncComponents now depends on
SyncClient, which now has a getter for a SyncApiComponentFactory.

BUG=512768, 512832
Committed: https://crrev.com/9276f21f78addeadc5179a2a3b2dcbd9835a5561
Cr-Commit-Position: refs/heads/master@{#346295}

[Nicolas Zea, 2015-08-27 00:26:15]

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

[Nicolas Zea, 2015-08-27 00:26:16]

+Stan for review, +Max FYI

Unfortunately this change touches all datatype controllers and their tests, as
well as a few other components, but all the changes are plumbing and refactoring
(and it's a net negative for lines of code!). PTAL

[stanisc, 2015-08-27 22:14:05]

Nice! LGTM with some nits/suggestions. Feel free addressing them in a separate
change.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:82: web_data_service_ =
GetWebDataService();
Since these two functions are called here and the return values are stored,
should they be protected rather than public?

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.h (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:42: Profile* const profile_;
I wonder if SyncClient should have a method that returns Profile since this
implementation gets Profile anyways? 
I noticed that Profile is often passed along with SyncClient, for example to
many data type controllers. Having Profile available from SyncClient would
simplify passing it around.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:43:
sync_driver::SyncApiComponentFactory* const component_factory_;
Consider adding a comment about ownership of component_factoring_

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:52: sync_driver::SyncService*
sync_service_;
Consider mentioning ownership of sync_service.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_components_factory_impl.cc:205:
sync_driver::SyncService* sync_service) {
Why does this have to receive sync_service? Could it get it from
chrome_sync_client_?

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_components_factory_impl.cc:219:
sync_driver::SyncService* sync_service) {
The same comment about sync_service as above.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_components_factory_impl.cc:290: // TODO(zea):
remove this once SyncedWindowDelegateGetter is componentized.
Should this have a crbug reference?

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service.h:475:
sync_driver::SyncApiComponentFactory* factory() { return factory_.get(); }
Should the function be const?

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
File components/sync_driver/non_ui_data_type_controller.h (right):

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/non_ui_data_type_controller.h:127: SyncClient* const
sync_client_;
Since DTC classes now have SyncClient it should be unnecessary in the base DTC
to have user_share() and OnUserShareReady(...).
Instead user share could be accessed via sync_client_ ->
GetSyncService()->GetUserShare(). That would also simplify changing the base DTC
to be used with USS data types. We could do that in a separate change.

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
File components/sync_driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/sync_api_component_factory.h:79: virtual void
Initialize(sync_driver::SyncService* pss) = 0;
not "pss" anymore

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/sync_api_component_factory.h:82: virtual void
RegisterDataTypes(sync_driver::SyncService* pss) = 0;
The same here

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/sync_service.h:223: virtual void
DeactivateDataType(syncer::ModelType type) = 0;
Right! I was going to do something like that to make DTC's compatible with the
new USS datatypes. Instead of SBH there should be a dedicated "registrar"
interface capable of handling both directory and non-directory data types.

[Nicolas Zea, 2015-08-27 23:37:14]

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

[Nicolas Zea, 2015-08-27 23:37:16]

Comments addressed (and removed some unnecessary #includes that were left
around). +Brett for options_util_unittest

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.cc (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.cc:82: web_data_service_ =
GetWebDataService();
On 2015/08/27 22:14:04, stanisc wrote:
> Since these two functions are called here and the return values are stored,
> should they be protected rather than public?

They're called externally too, but on the UI thread. It's only
GetSyncableServiceForType that needs to make use of them on a different thread.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
File chrome/browser/sync/chrome_sync_client.h (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:42: Profile* const profile_;
On 2015/08/27 22:14:04, stanisc wrote:
> I wonder if SyncClient should have a method that returns Profile since this
> implementation gets Profile anyways? 
> I noticed that Profile is often passed along with SyncClient, for example to
> many data type controllers. Having Profile available from SyncClient would
> simplify passing it around.

Most places that a profile is passed with the SyncClient will eventually have
the Profile removed once the file gets componentized. The purpose of the
SyncClient is to abstract away any dependency on the Profile, and therefore it
can't depend on any chrome/ components (although ChromeSyncClient can), but
we're only part way there.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:43:
sync_driver::SyncApiComponentFactory* const component_factory_;
On 2015/08/27 22:14:04, stanisc wrote:
> Consider adding a comment about ownership of component_factoring_

Done.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/ch...
chrome/browser/sync/chrome_sync_client.h:52: sync_driver::SyncService*
sync_service_;
On 2015/08/27 22:14:04, stanisc wrote:
> Consider mentioning ownership of sync_service.

Done.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_components_factory_impl.cc (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_components_factory_impl.cc:205:
sync_driver::SyncService* sync_service) {
On 2015/08/27 22:14:04, stanisc wrote:
> Why does this have to receive sync_service? Could it get it from
> chrome_sync_client_?

Done.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_components_factory_impl.cc:219:
sync_driver::SyncService* sync_service) {
On 2015/08/27 22:14:04, stanisc wrote:
> The same comment about sync_service as above.

Done.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_components_factory_impl.cc:290: // TODO(zea):
remove this once SyncedWindowDelegateGetter is componentized.
On 2015/08/27 22:14:04, stanisc wrote:
> Should this have a crbug reference?

Good point, done.

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
File chrome/browser/sync/profile_sync_service.h (right):

https://codereview.chromium.org/1310553005/diff/100001/chrome/browser/sync/pr...
chrome/browser/sync/profile_sync_service.h:475:
sync_driver::SyncApiComponentFactory* factory() { return factory_.get(); }
On 2015/08/27 22:14:04, stanisc wrote:
> Should the function be const?

Done.

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
File components/sync_driver/non_ui_data_type_controller.h (right):

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/non_ui_data_type_controller.h:127: SyncClient* const
sync_client_;
On 2015/08/27 22:14:04, stanisc wrote:
> Since DTC classes now have SyncClient it should be unnecessary in the base DTC
> to have user_share() and OnUserShareReady(...).
> Instead user share could be accessed via sync_client_ ->
> GetSyncService()->GetUserShare(). That would also simplify changing the base
DTC
> to be used with USS data types. We could do that in a separate change.

Acknowledged.

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
File components/sync_driver/sync_api_component_factory.h (right):

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/sync_api_component_factory.h:79: virtual void
Initialize(sync_driver::SyncService* pss) = 0;
On 2015/08/27 22:14:04, stanisc wrote:
> not "pss" anymore

Done.

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/sync_api_component_factory.h:82: virtual void
RegisterDataTypes(sync_driver::SyncService* pss) = 0;
On 2015/08/27 22:14:04, stanisc wrote:
> The same here

Removed this param.

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
File components/sync_driver/sync_service.h (right):

https://codereview.chromium.org/1310553005/diff/100001/components/sync_driver...
components/sync_driver/sync_service.h:223: virtual void
DeactivateDataType(syncer::ModelType type) = 0;
On 2015/08/27 22:14:05, stanisc wrote:
> Right! I was going to do something like that to make DTC's compatible with the
> new USS datatypes. Instead of SBH there should be a dedicated "registrar"
> interface capable of handling both directory and non-directory data types.

Interesting, yeah, I agree a registrar that handles this logic would be best
(likely accessed via the SyncService (or SyncClient). I'll update the comment
for now to mention that.

[brettw, 2015-08-28 19:34:32]

options util unittests lgtm ++

[Nicolas Zea, 2015-08-28 20:54:48]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-08-28 20:54:49]

The patchset sent to the CQ was uploaded after l-g-t-m from
stanisc@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1310553005/#ps200001
(title: "Fix android compile")

[commit-bot: I haz the power, 2015-08-28 20:55:06]

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

[commit-bot: I haz the power, 2015-08-28 21:49:18]

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

[commit-bot: I haz the power, 2015-08-28 21:49:19]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2015-08-28 22:24:55]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-08-28 22:24:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
stanisc@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1310553005/#ps180002
(title: "Fix GN deps")

[commit-bot: I haz the power, 2015-08-28 22:26:14]

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

[commit-bot: I haz the power, 2015-08-28 23:06:33]

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

[commit-bot: I haz the power, 2015-08-28 23:06:35]

Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2015-08-28 23:36:19]

The CQ bit was checked by zea@chromium.org

[Nicolas Zea, 2015-08-28 23:36:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
stanisc@chromium.org, brettw@chromium.org
Link to the patchset: https://codereview.chromium.org/1310553005/#ps250001
(title: "Really fix GN")

[commit-bot: I haz the power, 2015-08-28 23:37:06]

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

[commit-bot: I haz the power, 2015-08-29 00:44:03]

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

[commit-bot: I haz the power, 2015-08-29 00:44:04]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Nicolas Zea, 2015-08-29 00:51:37]

The CQ bit was checked by zea@chromium.org

[commit-bot: I haz the power, 2015-08-29 00:52:11]

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

[commit-bot: I haz the power, 2015-08-29 01:35:09]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-08-29 01:35:40]

Patchset 14 (id:??) landed as
https://crrev.com/9276f21f78addeadc5179a2a3b2dcbd9835a5561
Cr-Commit-Position: refs/heads/master@{#346295}