New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 882777 link

Starred by 1 user
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Dec 13
Cc:
mastiz@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocked on:
issue 883199
Blocking:
issue 867801
issue 906995



Sign in to add a comment

SyncClient should not expose SyncService

Project Member Reported by mastiz@chromium.org, Sep 11 
We currently have a cyclic dependency between SyncClient and SyncService, each having references to each other and the implementation following a weird initialization sequence, which happens to work because some code is executed lazily (e.g. GetSyncableServiceForType()).

Canonical "clients" in chromium are used to bundle and inject dependencies to a component (usually KeyedService, in this case ProfileSyncService), where the service itself should not be exposed. Contradicting this suggests a layering violation and requires ugly hacks.

Comment 1 by mastiz@chromium.org, Sep 12

Blockedon: 883199
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 12 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c015c9c2e2626a8b1533a790c826b9b2cdb568e9

commit c015c9c2e2626a8b1533a790c826b9b2cdb568e9
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Sep 12 19:58:37 2018

Remove SyncClient::Initialize()

There is no need for ProfileSyncService to initialize its own client,
which represents the dependencies it relies on. Instead, let the upper
layers do that, which allows simplifying interface SyncClient.

Bug:  882777 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: I877f1edabf85fd00d8a9b4b98a2d23af1885371c
Reviewed-on: https://chromium-review.googlesource.com/1218922
Reviewed-by: John Wu <jzw@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590798}
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/chrome/browser/sync/chrome_sync_client.h
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/chrome/browser/sync/profile_sync_service_factory.cc
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/chrome/browser/sync/profile_sync_service_factory.h
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/components/sync/driver/fake_sync_client.cc
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/components/sync/driver/fake_sync_client.h
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/components/sync/driver/sync_client.h
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/components/sync/driver/sync_client_mock.h
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/ios/chrome/browser/sync/ios_chrome_sync_client.h
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/ios/chrome/browser/sync/ios_chrome_sync_client.mm
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/ios/chrome/browser/sync/profile_sync_service_factory.cc
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/ios/web_view/internal/sync/web_view_profile_sync_service_factory.mm
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/ios/web_view/internal/sync/web_view_sync_client.h
[modify] https://crrev.com/c015c9c2e2626a8b1533a790c826b9b2cdb568e9/ios/web_view/internal/sync/web_view_sync_client.mm

Comment 3 by mastiz@chromium.org, Nov 13

Labels: -Pri-2 Pri-3

Comment 4 by mastiz@chromium.org, Dec 12

Blocking: 906995
Labels: -Pri-3 Pri-2
Project Member

Comment 5 by bugdroid1@chromium.org, Dec 13 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0b79a0a179b0f4e58cf284697fd1b9159e26069e

commit 0b79a0a179b0f4e58cf284697fd1b9159e26069e
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Dec 13 08:54:00 2018

Remove GetSyncService() from SyncClient

We have a weird cyclic dependency between sync controllers and the sync
service itself, because some datatypes (including most directory
datatypes) need interactions with ProfileSyncService.

Prior to this patch, the issue was hackily buried by exposing
SyncService in SyncClient, although the interface's purpose is to
inject dependencies for ProfileSyncService itself.

In the new approach, the dependency is made explicit in controllers'
constructors. This involves more boilerplate code but has one important
benefit: controllers have now access to SyncService at construction
time, which wasn't the case before because GetSyncService() couldn't be
called in the constructor (otherwise an infinite loop was entered).
This means that certain steps like registering a SyncServiceObserver
can be done right away, without artifically deferring it to an
arbitrary time (usually LoadModels()).

Bug:  882777 
Change-Id: I82c7ec783c42360cccf687daa016974c73f38a5c
Reviewed-on: https://chromium-review.googlesource.com/c/1373834
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Reviewed-by: Hiroshi Ichikawa <ichikawa@chromium.org>
Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#616249}
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/supervised_user/supervised_user_sync_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/supervised_user/supervised_user_sync_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/chrome_sync_client.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/glue/extension_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/glue/extension_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/glue/extension_setting_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/glue/extension_setting_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/glue/theme_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/sync/glue/theme_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/autofill_wallet_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/autofill_wallet_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/autofill_wallet_data_type_controller_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/autofill_wallet_model_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/autofill_wallet_model_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/webdata/autofill_profile_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/webdata/autofill_profile_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/webdata/autofill_profile_model_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/autofill/core/browser/webdata/autofill_profile_model_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/abstract_profile_sync_service_test.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/abstract_profile_sync_service_test.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_components_factory_impl.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_service_autofill_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/browser_sync/profile_sync_test_util.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/history/core/browser/sync/history_delete_directives_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/history/core/browser/sync/history_delete_directives_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/history/core/browser/sync/history_delete_directives_model_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/history/core/browser/sync/history_delete_directives_model_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/password_manager/core/browser/sync/password_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/password_manager/core/browser/sync/password_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/password_manager/core/browser/sync/password_model_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/password_manager/core/browser/sync/password_model_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/password_manager/core/browser/sync/password_syncable_service_based_model_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/password_manager/core/browser/sync/password_syncable_service_based_model_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/search_engines/search_engine_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/search_engines/search_engine_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/search_engines/search_engine_data_type_controller_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/async_directory_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/async_directory_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/async_directory_type_controller_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/directory_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/directory_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/fake_sync_client.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/fake_sync_client.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/frontend_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/frontend_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/frontend_data_type_controller_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/sync_api_component_factory.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/sync_api_component_factory_mock.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/sync_client.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync/driver/sync_client_mock.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync_bookmarks/bookmark_data_type_controller.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync_bookmarks/bookmark_data_type_controller.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/components/sync_bookmarks/bookmark_data_type_controller_unittest.cc
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/ios/chrome/browser/sync/ios_chrome_sync_client.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/ios/chrome/browser/sync/ios_chrome_sync_client.mm
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/ios/web_view/internal/sync/web_view_sync_client.h
[modify] https://crrev.com/0b79a0a179b0f4e58cf284697fd1b9159e26069e/ios/web_view/internal/sync/web_view_sync_client.mm

Comment 6 by mastiz@chromium.org, Dec 13

Status: Fixed (was: Started)
►

Sign in to add a comment