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

Issue 819993 link

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

Blocking:
issue 516866



Sign in to add a comment

Remove all dependencies to ModelTypeSyncBridge except in ClientTagBasedModelTypeProcessor

Project Member Reported by mastiz@chromium.org, Mar 8 2018 
We conceptually want the bridge to be a delegate to ClientTagBasedModelTypeProcessor, because bookmarks is not planning to adopt this interface.

Independently of that real need, hiding the complexity of the bridge will anyway be a benefit and an improved design (right now the bridge interface mixes various roles).

This will likely involve introducing a new interface that supports:
- OnSyncStarting(), possibly renamed to StartSync().
- DisableSync().
- GetAllNodes(), possibly renamed to GetAllNodesForDebug(); or alternatively the combination of GetAllData()+GetAllMetadata() as is the case today (very ugly).

It might be preferable that the last above is moved somewhere else, either ModelTypeProcessor interface or a dedicated interface for debugging.

Comment 2 by mamir@chromium.org, Mar 9 2018 

This comment should also be fixed
https://cs.chromium.org/chromium/src/components/sync/model/entity_data.h?gsn=UpdateResponseDataList&l=32

Local EntityData creation isn't limited to the ModelTypeSyncBridge.

Comment 3 by mastiz@chromium.org, Mar 21 2018

Cc: mastiz@chromium.org jkrcal@chromium.org

Comment 4 by mastiz@chromium.org, Mar 21 2018 

jkrcal@: This bug reflects what we've discussed today. There is some dependency to our plans for DisableSync(), specially if we agree that DisableSync() is only called when the model is ready to sync (then, one option would be to just list it in ModelTypeProcessor). 

Also, as one more code pointer: the functions being discussed here are used by ModelTypeController. This is a codebase that we should understand well before moving forward.

Comment 5 by jkrcal@chromium.org, Apr 18 2018

Cc: -jkrcal@chromium.org mamir@chromium.org
Owner: jkrcal@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Apr 24 2018 

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

commit 801ab7f60949030814e75c4a7b5f3de51903ca60
Author: Jan Krcal <jkrcal@chromium.org>
Date: Tue Apr 24 17:03:50 2018

[USS] Split ModelTypeControllerDelegate from ModelTypeSyncBridge

This CL splits the part of the Bridge's interface that is used by
ModelTypeController into a separate Delegate interface. For the reasons
of a smooth transition for follow-up CLs, the Bridge inherits from this
new Delegate.

In the future, we will make the Processor extend this new interface
instead of the Bridge.

Bug:  819993 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: Ieacafd872a16f2f587965c1710c6abacb3517efc
Reviewed-on: https://chromium-review.googlesource.com/1007134
Reviewed-by: Roger McFarlane <rogerm@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553168}
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/chrome/browser/sync/chrome_sync_client.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/autofill/core/browser/webdata/web_data_model_type_controller.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/autofill/core/browser/webdata/web_data_model_type_controller.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/history/core/browser/typed_url_model_type_controller.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/history/core/browser/typed_url_model_type_controller.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/driver/fake_sync_client.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/driver/fake_sync_client.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/driver/sync_client.h
[add] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/model/model_type_controller_delegate.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/model/model_type_debug_info.cc
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/model/model_type_debug_info.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/ios/chrome/browser/sync/ios_chrome_sync_client.h
[modify] https://crrev.com/801ab7f60949030814e75c4a7b5f3de51903ca60/ios/chrome/browser/sync/ios_chrome_sync_client.mm
Project Member

Comment 7 by bugdroid1@chromium.org, May 8 2018 

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

commit 970fd082eaf05d394eb400286af2acb64a300e5b
Author: Jan Krcal <jkrcal@chromium.org>
Date: Tue May 08 14:36:21 2018

[USS] Uncouple ModelTypeControllerDelegate from bridges - part 1

This CL unblocks
https://chromium-review.googlesource.com/c/chromium/src/+/102589 that
separates the controller delegate interface from the bridges and makes
ClientTagBasedModelTypeProcessor directly implement the interface.

Since the processor needs to be able to inform the bridge that sync
is starting (before the bridge is ready to sync, i.e. before it gets
the pointer to the bridge), this CL passes the (preliminary) bridge
pointer to the processor during construction of the bridge.

This CL is admittedly hacky, it only minimizes the CL size and thus
the speed for landing the blocked CL. After the CL gets landed, we
need to remove this hack and introduce a proper solution, probably by
swapping the ownership of the bridge and the processor.

Bug:  819993 
Change-Id: Ie1950e6626a3ab58a583d8fbe502ba52c34319e0
Reviewed-on: https://chromium-review.googlesource.com/1042707
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556793}
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model/fake_model_type_change_processor.cc
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model/fake_model_type_change_processor.h
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model/mock_model_type_change_processor.cc
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model/mock_model_type_change_processor.h
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/970fd082eaf05d394eb400286af2acb64a300e5b/components/sync_sessions/session_sync_bridge_unittest.cc
Project Member

Comment 8 by bugdroid1@chromium.org, May 16 2018 

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

commit f894cc426ea9aa86683c6ef16f324c907bd16c46
Author: Jan Krcal <jkrcal@chromium.org>
Date: Wed May 16 10:03:43 2018

[USS] Uncouple ModelTypeControllerDelegate from bridges - part 2

This CL moves the ModelTypeControllerDelegate interface out of the
ModelTypeSyncBridge interface. The interface is now directly
implemented by ClientTagBasedModelTypeProcessor wich is the more
natural thing to do as most of the functionality is anyway already
implemented there.

This is part of the bigger effort to hide the bridge interface from the
internals of sync.

Bug:  819993 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I74152497d46a389d127ca09a3678510b6a0efb9d
Reviewed-on: https://chromium-review.googlesource.com/1025893
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559028}
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/chrome/browser/sync/chrome_sync_client.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/browser_sync/profile_sync_components_factory_impl.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/history/core/browser/typed_url_model_type_controller.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/BUILD.gn
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/device_info/device_info_sync_bridge_unittest.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/driver/fake_sync_client.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/driver/model_type_controller.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/fake_model_type_change_processor.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/fake_model_type_change_processor.h
[add] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/fake_model_type_controller_delegate.cc
[add] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/fake_model_type_controller_delegate.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/fake_model_type_sync_bridge.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/mock_model_type_change_processor.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/mock_model_type_change_processor.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/model_type_controller_delegate.h
[delete] https://crrev.com/64059965656108b4d6a08c5b708924c2a917f58b/components/sync/model/model_type_debug_info.cc
[delete] https://crrev.com/64059965656108b4d6a08c5b708924c2a917f58b/components/sync/model/model_type_debug_info.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/model_type_sync_bridge_unittest.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model/stub_model_type_sync_bridge.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync/user_events/user_event_sync_bridge_unittest.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync_sessions/session_sync_bridge.h
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/components/sync_sessions/session_sync_bridge_unittest.cc
[modify] https://crrev.com/f894cc426ea9aa86683c6ef16f324c907bd16c46/ios/chrome/browser/sync/ios_chrome_sync_client.mm

Comment 9 by jkrcal@chromium.org, May 16 2018 

The next step is to cleanup properly after the commit from #7:
 - either remove Bridge* from ModelReadyToSync
 - or swap the ownership of the bridge and the processor.

The former is probably considerably less work.
Project Member

Comment 10 by bugdroid1@chromium.org, May 19 2018 

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

commit d7033aa4dbc9970425c08563da44b7c2e67cb04a
Author: Jan Krcal <jkrcal@chromium.org>
Date: Sat May 19 21:47:05 2018

[USS] Remove bridge pointer from ModelReadyToSync

This CL removes the bridge pointer from the ModelReadyToSync function of
the ModelTypeChangeProcessor interface.The CL finishes the larger effort
of hiding the bridge interface from the internals of sync.

When already touching all the files, it also removes the gMock hacks for
move-only types from MockModelTypeChangeProcessor. The outcome is still
not optimal because some standard actions as Invoke or SaveArg do not
support move-only types; we have to use lambdas/WithArg, instead.

Bug:  819993 
Change-Id: I0f4db6c2b7b36d317b69a25a651180c1f63d9269
Reviewed-on: https://chromium-review.googlesource.com/1064612
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Sebastien Seguin-Gagnon <sebsg@chromium.org>
Reviewed-by: Eric Noyau <noyau@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#560173}
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/chrome/browser/chromeos/printing/printers_sync_bridge.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/history/core/browser/typed_url_sync_bridge.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/reading_list/core/reading_list_store.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/device_info/device_info_sync_bridge.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/device_info/device_info_sync_bridge_unittest.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model/fake_model_type_change_processor.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model/fake_model_type_change_processor.h
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model/mock_model_type_change_processor.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model/mock_model_type_change_processor.h
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model/recording_model_type_change_processor.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model/recording_model_type_change_processor.h
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync/user_events/user_event_sync_bridge_unittest.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/d7033aa4dbc9970425c08563da44b7c2e67cb04a/components/sync_sessions/session_sync_bridge_unittest.cc

Comment 11 by jkrcal@chromium.org, May 28 2018 

Mohamed, I do not understand what you mean by #2.
Apart from that, I think the bug is fixed.
Would you mind me closing the bug?

Comment 12 by mamir@chromium.org, May 28 2018 

What I was proposing is to rephrase the comment in EntityData.
It reads 

// A light-weight container for sync entity data which represents either
// local data created on the ModelTypeSyncBridge side ...

However, EntityData could be created on non-bridge types such as bookmarks.

Something like that would do the job I think. I am not sure.

// A light-weight container for sync entity data which represents either
// local data created on the local model side ...

Otherwise, closing the bug SGTM.
Project Member

Comment 13 by bugdroid1@chromium.org, May 29 2018 

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

commit 03d5a7880dd33ae9237cbcfb9ba35cbbc130a637
Author: Jan Krcal <jkrcal@chromium.org>
Date: Tue May 29 07:29:59 2018

[USS] Rephrase the comment for EntityData

The CL rephrases a comment for EntityData to remove the reference to the
ModelTypeSyncBridge which is an implementation detail of each model
type.

Bug:  819993 
Change-Id: I966330419913f70d7a4bbf70567727a4ace7283c
Reviewed-on: https://chromium-review.googlesource.com/1075531
Reviewed-by: Mohamed Amir Yosef <mamir@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562360}
[modify] https://crrev.com/03d5a7880dd33ae9237cbcfb9ba35cbbc130a637/components/sync/model/entity_data.h

Comment 14 by jkrcal@chromium.org, May 29 2018

Status: Fixed (was: Started)
Hurray!
►

Sign in to add a comment