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

Issue 807621 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[USS] Swap the order of initialization of ModelTypeSyncBridge and ModelTypeChangeProcessor

Project Member Reported by jkrcal@chromium.org, Jan 31 2018

Issue description

The current initialization sequence is: 
 1) the factory of the model runs:
      processor_factory_callback =
          base::BindRepeating(&ModelTypeChangeProcessor::Create, ...);
      bridge_ = std::make_unique<ModelXYZSyncBridge>
                    (processor_factory_callback, ...);

 2) the bridge runs:
      processor_ = processor_factory_callback.Run(this, ...);

 3) after all the initialization of the model is done, the bridge calls
      processor_->ModelReadyToSync(...);

We propose the following sequence
 1) the factory of the model runs 
      processor = ModelTypeChangeProcessor::Create(...);
      bridge_ = std::make_unique<ModelXYZSyncBridge>
                    (std::move(processor), ...);

 2) after all the initialization of the model is done, the bridge calls
      processor_->ModelReadyToSync(this, ...);

This way, the API is safer and the state space of the processor is simpler as the processor simply cannot call the bridge before the model is ready.

The only caveat is that currently, the bridge resets the processor whenever sync gets disabled (using the provided repeated "factory" callback). We would need to introduce
    virtual void ModelTypeChangeProcessor::Reset();
that resets the internal state of the processor (apart from the pointer to the bridge).
 

Comment 1 by mastiz@chromium.org, Jan 31 2018

Here's why I think the caveat doesn't apply: this bug's scope is one step among multiple to make ModelTypeSyncBridge an implementation detail of the datatype that is not exposed anywhere other than the corresponding processor.

In that world, DisableSync() would hit directly the processor (probably via interface  ModelTypeProcessor).

Meanwhile, before we do any of that, we can probably implement the Reset() logic you mention inside the processor's DisableSync(), without extending the interface, right?

Comment 2 by jkrcal@chromium.org, Jan 31 2018

#1: Yes, you are right, the logic can be implemented in DisableSync(), I've overlooked it.
Project Member

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

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

commit 85e25f04b6697e5c99346efa344b6545a734c24a
Author: Jan Krcal <jkrcal@chromium.org>
Date: Wed Mar 21 11:40:46 2018

[USS] Refactor initialization of Processor and Bridge

The CL takes one step in the direction to make ModelTypeSyncBridge an
implementation detail of the datatype that is not exposed anywhere else
than to the corresponding processor.

This CL simplifies the initialization of the Processor and the Bridge
and also pulls the factory function into the implementation
(ClientTagBasedModelTypeProcessor).

Bug:  807621 
Cq-Include-Trybots: master.tryserver.chromium.mac:ios-simulator-cronet;master.tryserver.chromium.mac:ios-simulator-full-configs
Change-Id: I68d1c79551bc9c86c9b62138c9cc7dae52b23f21
Reviewed-on: https://chromium-review.googlesource.com/923982
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Olivier Robin <olivierrobin@chromium.org>
Reviewed-by: Evan Stade <estade@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Pavel Yatsuk <pavely@chromium.org>
Commit-Queue: Jan Krcal <jkrcal@chromium.org>
Cr-Commit-Position: refs/heads/master@{#544670}
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/chrome/browser/chromeos/printing/printers_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/chrome/browser/sync/user_event_service_factory.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/autofill/core/browser/webdata/autocomplete_sync_bridge.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/history/core/browser/history_backend.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/history/core/browser/typed_url_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/history/core/browser/typed_url_sync_bridge.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/history/core/browser/typed_url_sync_bridge_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/reading_list/core/reading_list_model_storage.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/reading_list/core/reading_list_model_storage.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/reading_list/core/reading_list_model_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/reading_list/core/reading_list_store.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/reading_list/core/reading_list_store.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/reading_list/core/reading_list_store_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/device_info/device_info_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/device_info/device_info_sync_bridge.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/device_info/device_info_sync_bridge_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/fake_model_type_change_processor.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/fake_model_type_change_processor.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/fake_model_type_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/fake_model_type_sync_bridge.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/mock_model_type_change_processor.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/mock_model_type_change_processor.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/model_type_change_processor.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/model_type_change_processor.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/model_type_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/model_type_sync_bridge.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/model_type_sync_bridge_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/recording_model_type_change_processor.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/recording_model_type_change_processor.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/stub_model_type_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model/stub_model_type_sync_bridge.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model_impl/client_tag_based_model_type_processor.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/user_events/user_event_service_impl_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/user_events/user_event_sync_bridge.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/user_events/user_event_sync_bridge.h
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/components/sync/user_events/user_event_sync_bridge_unittest.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/ios/chrome/browser/reading_list/reading_list_model_factory.cc
[modify] https://crrev.com/85e25f04b6697e5c99346efa344b6545a734c24a/ios/chrome/browser/sync/ios_user_event_service_factory.cc

Comment 4 by jkrcal@chromium.org, Apr 10 2018

Status: Fixed (was: Assigned)

Sign in to add a comment