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

Issue 865936 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jul 24
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

Cleanup sync_service.h

Project Member Reported by treib@chromium.org, Jul 20

Issue description

The SyncService interface exposes around 50 methods, and currently they're listed in no particular order: State queries, modifications, debug-UI-only getters, etc etc are all arbitrarily mixed. This makes the header hard to read.
 
Some more thoughts:
- There are a bunch of "add 1 to i"-level comments around.
- Conversely, some non-trivial methods are lacking a comment.
- As a slightly larger follow-up, we could consider moving debug-UI-only functionality from SyncService down into ProfileSyncService. This would require updating SyncInternalsMessageHandler to use ProfileSyncService rather than SyncService, which should be trivial.
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 20

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

commit 3081884fd7164ced3d12510ba55e1813fe301b9f
Author: Marc Treib <treib@chromium.org>
Date: Fri Jul 20 13:43:12 2018

Cleanup: Move ClearServerDataEvents from sync_service.h into its own file

This is an enum used interally for metrics. Clients of SyncService don't
need to know of it, so no reason to pollute the (already too big)
sync_service.h file.

Bug:  865936 
Change-Id: Ia4a3a65a285e7db61130944340a355460971379b
Reviewed-on: https://chromium-review.googlesource.com/1145196
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Cr-Commit-Position: refs/heads/master@{#576843}
[modify] https://crrev.com/3081884fd7164ced3d12510ba55e1813fe301b9f/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/3081884fd7164ced3d12510ba55e1813fe301b9f/components/sync/BUILD.gn
[add] https://crrev.com/3081884fd7164ced3d12510ba55e1813fe301b9f/components/sync/driver/clear_server_data_events.h
[modify] https://crrev.com/3081884fd7164ced3d12510ba55e1813fe301b9f/components/sync/driver/sync_service.h
[modify] https://crrev.com/3081884fd7164ced3d12510ba55e1813fe301b9f/components/sync/driver/sync_service_crypto.cc

Status: Started (was: Assigned)
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 24

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

commit e7c91b4fd0c3726a6a747a610758ae95f001361a
Author: Marc Treib <treib@chromium.org>
Date: Tue Jul 24 10:24:34 2018

Cleanup sync_service.h: Reordering and comments

Before this CL, all the methods in sync_service.h seemed to be in a
totally arbitrary order. This CL attempts to reorder them to be grouped
in a logical way, and also updates some comments that were out of date.
No functional changes.

Bug:  865936 
Change-Id: Ief7c8e8ac6f4eff0e1002dae54df1e96b5e7e00b
Reviewed-on: https://chromium-review.googlesource.com/1146646
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577475}
[modify] https://crrev.com/e7c91b4fd0c3726a6a747a610758ae95f001361a/components/sync/driver/sync_service.h

Status: Fixed (was: Started)
Let's call this one done. As always, there's plenty of additional cleanup that could be done.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 15

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

commit 3b26a2220dcb63b58002944bde39e3fcd69c5dde
Author: Marc Treib <treib@chromium.org>
Date: Mon Oct 15 11:24:57 2018

Move GetSyncClient from SyncService down into ProfileSyncService

Also add a ForTest suffix since it's only used in tests.

Bug:  865936 
Change-Id: I207e72a19905d77003aee1d9a4d951bbeaed105d
Reviewed-on: https://chromium-review.googlesource.com/c/1280428
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599612}
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/chrome/browser/sync/test/integration/bookmarks_helper.cc
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/chrome/browser/sync/test/integration/single_client_directory_sync_test.cc
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/components/browser_sync/abstract_profile_sync_service_test.cc
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/3b26a2220dcb63b58002944bde39e3fcd69c5dde/components/sync/driver/sync_service.h

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 16

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

commit 7328e56cf3bb18169369cd710a76707a1289960a
Author: Marc Treib <treib@chromium.org>
Date: Tue Oct 16 06:35:29 2018

Remove PassphraseType param from SyncService::SetEncryptionPassphrase

The passphrase type had two possible values, EXPLICIT and IMPLICIT.
IMPLICIT is an ancient legacy concept that hasn't been used in many
years, apart from tests: ProfileSyncServiceHarness did use it in some
circumstances. It's a bad idea to have tests go through an
otherwise-dead code path, so this CL removes those calls.

Bug:  865936 
Cq-Include-Trybots: luci.chromium.try:ios-simulator-cronet;luci.chromium.try:ios-simulator-full-configs
Change-Id: Ib91570265eccbe8831e4c831ad9d5dc3c7a6cc18
Reviewed-on: https://chromium-review.googlesource.com/c/1280268
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: David Roger <droger@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Steven Holte <holte@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599891}
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/metrics/ukm_browsertest.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/sync/profile_sync_service_android.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/sync/test/integration/profile_sync_service_harness.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/sync/test/integration/single_client_custom_passphrase_sync_test.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/sync/test/integration/two_client_passwords_sync_test.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/ui/webui/settings/people_handler.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/chrome/browser/ui/webui/settings/people_handler_unittest.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/components/browser_sync/profile_sync_service_mock.h
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/components/sync/driver/fake_sync_service.cc
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/components/sync/driver/fake_sync_service.h
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/components/sync/driver/sync_service.h
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/ios/chrome/browser/ui/settings/sync_create_passphrase_collection_view_controller_unittest.mm
[modify] https://crrev.com/7328e56cf3bb18169369cd710a76707a1289960a/ios/chrome/browser/ui/settings/sync_encryption_passphrase_collection_view_controller.mm

Project Member

Comment 8 by bugdroid1@chromium.org, Oct 16

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

commit f211549db745cd0d2004699e0c7b681f1067db79
Author: Marc Treib <treib@chromium.org>
Date: Tue Oct 16 06:58:47 2018

Sync cleanup: Remove handling/wiring for implicit passphrase

Follow-up to https://crrev.com/c/1280268: Setting an implicit passphrase
has not been supported for a long time. This CL removes a bunch of code
to handle that case, and removes the accompanying tests.

Bug:  865936 
Change-Id: I3a7ff70b06ebebde8a684dfa5343b3dba563099b
Reviewed-on: https://chromium-review.googlesource.com/c/1280724
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#599894}
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/driver/glue/sync_backend_host_core.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/driver/glue/sync_backend_host_core.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/driver/glue/sync_backend_host_impl.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/driver/glue/sync_backend_host_impl.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/driver/sync_service_crypto.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/driver/sync_service_crypto.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine/fake_sync_engine.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine/fake_sync_engine.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine/mock_sync_engine.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine/sync_encryption_handler.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine/sync_engine.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine_impl/sync_encryption_handler_impl.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine_impl/sync_encryption_handler_impl.h
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine_impl/sync_encryption_handler_impl_unittest.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/engine_impl/sync_manager_impl_unittest.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/test/fake_sync_encryption_handler.cc
[modify] https://crrev.com/f211549db745cd0d2004699e0c7b681f1067db79/components/sync/test/fake_sync_encryption_handler.h

Sign in to add a comment