Rethink ProfileSyncService vs SyncServiceBase split |
|||
Issue descriptionThe current state of affairs is: - SyncServiceBase is actually a partial implementation of the SyncService interface. - ProfileSyncService is the only full implementation; nobody uses SyncServiceBase. - The split is very weak: SyncServiceBase has lots of protected member variables which ProfileSyncService uses directly. Apparently the idea was to move as much as possible into components/sync/, but it’s not clear to me how that is better than components/browser_sync/ (where PSS lives). Looks like a now-obsolete refactoring that was abandoned halfway through?
,
May 4 2018
+1 having a class implementation spreading across different components is a very strong smell. I'd vote for moving the SyncServiceBase functionality into the ProfileSyncService -- it's mainly about initialization and life-cycle management. IMO, that should be the main responsibility of ProfileSyncServce. ProfileSyncService has a lot of functionality about random other things (mostly for the long list of observers it implements) which instead should be moved into separate classes. A good example is the whole Access token handling which is a completely independent piece of functionality. I don't want to unnecessarily expand the scope of this bug, but there are much better candidates on how the functionality could be split than the current one ;-)
,
May 4 2018
The access token/refresh token management will hopefully just get a lot simpler soon, in the course of bug 825190 . But generally I agree, there are many things that should get pulled out of PSS, but the current SyncServiceBase just isn't one :)
,
May 4 2018
,
May 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a commit 7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a Author: Marc Treib <treib@chromium.org> Date: Mon May 07 11:59:24 2018 Cleanup: Merge SyncServiceBase back into ProfileSyncService As far as I can tell, the current split was a started but abandoned refactoring. As it is, the two classes are very tightly coupled (PSS directly uses many protected members of SSB), and it's entirely unclear what the intended split of responsibilities is. So let's just merge the two back into one. TBRing trivial change in recent_tabs_sub_menu_model.h TBR=ellyjones Bug: 839823 Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs Change-Id: I0dbdf9115c3359dca2cd05bde55ae9d24a081223 Reviewed-on: https://chromium-review.googlesource.com/1044219 Commit-Queue: Marc Treib <treib@chromium.org> Reviewed-by: Tim Schumann <tschumann@chromium.org> Cr-Commit-Position: refs/heads/master@{#556418} [modify] https://crrev.com/7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h [modify] https://crrev.com/7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a/components/browser_sync/profile_sync_service.h [modify] https://crrev.com/7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a/components/browser_sync/profile_sync_service_unittest.cc [modify] https://crrev.com/7f65b0c0e07ba9263ad8b82d09d4ab267177cd1a/components/sync/BUILD.gn [delete] https://crrev.com/003ff60f10c74b2bf95bfdc5b749faca6fb66050/components/sync/driver/sync_service_base.cc [delete] https://crrev.com/003ff60f10c74b2bf95bfdc5b749faca6fb66050/components/sync/driver/sync_service_base.h
,
May 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b8bc2ac36e12e1eda488295dbf4f9ee643f63b1f commit b8bc2ac36e12e1eda488295dbf4f9ee643f63b1f Author: Marc Treib <treib@chromium.org> Date: Mon May 07 13:17:05 2018 ProfileSyncService: use THREAD_CHECKER macros instead of using the ThreadChecker class directly. This way, it'll only be instantiated in DCHECK-enabled builds. Bug: 839823 Change-Id: Ifcbbc4113265fd0067a1df10fc4a26ca844139f1 Reviewed-on: https://chromium-review.googlesource.com/1046845 Reviewed-by: Tim Schumann <tschumann@chromium.org> Commit-Queue: Marc Treib <treib@chromium.org> Cr-Commit-Position: refs/heads/master@{#556423} [modify] https://crrev.com/b8bc2ac36e12e1eda488295dbf4f9ee643f63b1f/components/browser_sync/profile_sync_service.cc [modify] https://crrev.com/b8bc2ac36e12e1eda488295dbf4f9ee643f63b1f/components/browser_sync/profile_sync_service.h
,
May 7 2018
Done as specified :) As per comment #2, there's clearly a lot of cleanup/splitting that should be done to PSS, but let's file separate bugs for that. |
|||
►
Sign in to add a comment |
|||
Comment 1 by treib@chromium.org
, May 4 2018