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

Issue 895455 link

Starred by 2 users
Status: Fixed
Owner:
mastiz@chromium.org
Closed: Nov 29
Cc:
mastiz@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Task

Blocked on:
issue 681921


Sign in to add a comment

Post-launch code cleanup for SESSIONS

Project Member Reported by mastiz@chromium.org, Oct 15 
SESSIONS USS has been rolled out to all users for two releases, and we're past a third branch point, so it's time to clean the old code up. This includes, at least:
- SessionSyncManager
- AbstractSessionsSyncManager
- kSyncUSSSessions (feature toggle)

There are some unit tests that will require updating too, in particular recent_tabs_sub_menu_model_unittest.cc.
Project Member

Comment 2 by bugdroid1@chromium.org, Nov 16 

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

commit 72bc0424e38e0b6b92d477160e6cd4fa845b7676
Author: Mikel Astiz <mastiz@chromium.org>
Date: Fri Nov 16 03:02:36 2018

Avoid test dependency to SESSIONS SyncableService

These unit tests are the last examples of tests depending on
implementation details of SESSIONS sync, where overriding a feature
toggle was necessary.

Instead, let's embrace the new USS implementation.

Longer term, it may make sense to remove this dependency altogether and
migrate these unit tests to a test-specific OpenTabsUIDelegate. For
anything fancier, we should use proper sync integration tests. This
intermediate step makes it more obvious that there aren't behavioral
changes between the two sync implementations.

Bug:  895455 
Change-Id: I1c3f26ec3037d71908cbf090644bf0580d8fd41b
Reviewed-on: https://chromium-review.googlesource.com/c/1301435
Commit-Queue: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608641}
[modify] https://crrev.com/72bc0424e38e0b6b92d477160e6cd4fa845b7676/chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.cc
[modify] https://crrev.com/72bc0424e38e0b6b92d477160e6cd4fa845b7676/chrome/browser/ui/toolbar/recent_tabs_builder_test_helper.h
[modify] https://crrev.com/72bc0424e38e0b6b92d477160e6cd4fa845b7676/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
[modify] https://crrev.com/72bc0424e38e0b6b92d477160e6cd4fa845b7676/components/sync_sessions/session_sync_service.cc
[modify] https://crrev.com/72bc0424e38e0b6b92d477160e6cd4fa845b7676/components/sync_sessions/session_sync_service.h

Comment 3 by mastiz@chromium.org, Nov 16

Labels: -Pri-3 sync-fixit-2018q4 Pri-2
Status: Started (was: Assigned)
Removal of code should be unblocked and worth doing during the upcoming fixit week.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 19 

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

commit 15ec33029e98655954d4ce2ab3cdf67e2f7577a1
Author: Mikel Astiz <mastiz@chromium.org>
Date: Mon Nov 19 10:44:59 2018

Remove legacy SESSIONS sync implementation

The SyncableService (SessionsSyncManager), the directory datatype
controller and all abstractions required for accomodating two
implementations are removed, together with the corresponding about flag
and feature toggle (kill switch).

Sync integration tests are updated because they also exercised the
legacy codepath.

TBR=rohitrao@chromium.org,pkasting@chromium.org

Bug:  895455 
Change-Id: Ifaca1696e97f88b70b0c61ad89fed2372b239e46
Reviewed-on: https://chromium-review.googlesource.com/c/1340814
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609234}
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/chrome/browser/about_flags.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/chrome/browser/sync/chrome_sync_client.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/chrome/browser/sync/test/integration/single_client_sessions_sync_test.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/browser_sync/profile_sync_components_factory_impl.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/browser_sync/profile_sync_service_autofill_unittest.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/browser_sync/profile_sync_service_startup_unittest.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/browser_sync/profile_sync_service_unittest.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/fake_sync_client.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/fake_sync_client.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/sync_api_component_factory.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/sync_api_component_factory_mock.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/sync_client.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/sync_client_mock.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/sync_driver_switches.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync/driver/sync_driver_switches.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync_sessions/BUILD.gn
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/abstract_sessions_sync_manager.cc
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/abstract_sessions_sync_manager.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync_sessions/lost_navigations_recorder_unittest.cc
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/session_data_type_controller.cc
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/session_data_type_controller.h
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/session_data_type_controller_unittest.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync_sessions/session_sync_bridge.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync_sessions/session_sync_service.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/components/sync_sessions/session_sync_service.h
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/sessions_sync_manager.cc
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/sessions_sync_manager.h
[delete] https://crrev.com/0fd2049fc855126eb6f3c6ac963b7ecfaba014c2/components/sync_sessions/sessions_sync_manager_unittest.cc
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/ios/chrome/browser/sync/ios_chrome_sync_client.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/ios/chrome/browser/sync/ios_chrome_sync_client.mm
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/ios/web_view/internal/sync/web_view_sync_client.h
[modify] https://crrev.com/15ec33029e98655954d4ce2ab3cdf67e2f7577a1/ios/web_view/internal/sync/web_view_sync_client.mm
Project Member

Comment 5 by bugdroid1@chromium.org, Nov 20 

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

commit 05c9349e14bcd6f08104278e20f526a86bad2026
Author: Mikel Astiz <mastiz@chromium.org>
Date: Tue Nov 20 15:49:32 2018

Remove modification timestamp from SyncData

The legacy (pre-USS) implementation of session sync was the only
remaining user of this field and has been deleted recently, so let's
just remove the now unused API.

This is convenient for pseudo-USS (SyncableServiceBasedBridge) because
there is no need for additional plumbing to propagate this information
through (it wasn't actually properly populated prior to this patch).

Bug:  895455 
Change-Id: I02a577978b5fd7a84ced4775cd200d0210928985
Reviewed-on: https://chromium-review.googlesource.com/c/1343003
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609715}
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/chrome/browser/chromeos/preferences_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/chrome/browser/extensions/extension_disabled_ui_browsertest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/chrome/browser/prefs/synced_pref_change_registrar_browsertest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/chrome/browser/themes/theme_syncable_service_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/chrome/browser/ui/app_list/app_list_syncable_service_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/dom_distiller/core/dom_distiller_store_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/history/core/browser/history_service_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync/driver/generic_change_processor.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync/model/sync_change_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync/model/sync_data.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync/model/sync_data.h
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync/model/sync_data_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync/model_impl/syncable_service_based_bridge.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync/model_impl/syncable_service_based_bridge_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync_preferences/pref_service_syncable_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync_sessions/favicon_cache_unittest.cc
[modify] https://crrev.com/05c9349e14bcd6f08104278e20f526a86bad2026/components/sync_wifi/wifi_credential_syncable_service_unittest.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 21 

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

commit 90097b38218525d8a0123f63f0587bee6bc44736
Author: Mikel Astiz <mastiz@chromium.org>
Date: Wed Nov 21 19:49:38 2018

Split SessionSyncService and Impl

This will allow tests to introduce doubles in future patches.

Bug:  895455 
Change-Id: I33b99a8a90f00ab47678a852e4521e90deee1390
Reviewed-on: https://chromium-review.googlesource.com/c/1346453
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610181}
[modify] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/chrome/browser/sync/session_sync_service_factory.cc
[modify] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model_unittest.cc
[modify] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/components/sync_sessions/BUILD.gn
[modify] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/components/sync_sessions/session_sync_service.cc
[modify] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/components/sync_sessions/session_sync_service.h
[add] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/components/sync_sessions/session_sync_service_impl.cc
[add] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/components/sync_sessions/session_sync_service_impl.h
[modify] https://crrev.com/90097b38218525d8a0123f63f0587bee6bc44736/ios/chrome/browser/sync/session_sync_service_factory.cc
Project Member

Comment 7 by bugdroid1@chromium.org, Nov 29 

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

commit cbf0f912e891c1b3f192317f08f3e70a74c6fd4b
Author: Mikel Astiz <mastiz@chromium.org>
Date: Thu Nov 29 06:56:52 2018

Simplify ios session sync logic

SessionSyncService's API is sufficient to achieve the desired
behavior, i.e. distinguish the exact state for tab sync: off,
initial download in progress and done. It also issues a
notification every time that state changes, so there's no need to
monitor updates from ProfileSyncService directly.

Bug:  895455 
Change-Id: Icf66328fdde97f833e178f0571dedb248c91beca
Reviewed-on: https://chromium-review.googlesource.com/c/1344029
Reviewed-by: Sergio Collazos <sczs@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#612077}
[modify] https://crrev.com/cbf0f912e891c1b3f192317f08f3e70a74c6fd4b/ios/chrome/browser/sync/sync_setup_service.cc
[modify] https://crrev.com/cbf0f912e891c1b3f192317f08f3e70a74c6fd4b/ios/chrome/browser/sync/sync_setup_service.h
[modify] https://crrev.com/cbf0f912e891c1b3f192317f08f3e70a74c6fd4b/ios/chrome/browser/ui/recent_tabs/BUILD.gn
[modify] https://crrev.com/cbf0f912e891c1b3f192317f08f3e70a74c6fd4b/ios/chrome/browser/ui/recent_tabs/recent_tabs_coordinator_unittest.mm
[modify] https://crrev.com/cbf0f912e891c1b3f192317f08f3e70a74c6fd4b/ios/chrome/browser/ui/recent_tabs/recent_tabs_mediator.mm
[modify] https://crrev.com/cbf0f912e891c1b3f192317f08f3e70a74c6fd4b/ios/chrome/browser/ui/recent_tabs/synced_sessions_bridge.h
[modify] https://crrev.com/cbf0f912e891c1b3f192317f08f3e70a74c6fd4b/ios/chrome/browser/ui/recent_tabs/synced_sessions_bridge.mm

Comment 8 by mastiz@google.com, Nov 29

Status: Fixed (was: Started)
►

Sign in to add a comment