Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1763953002:
[USS] Change the place where SharedModelTypeProcessor got created (Closed)
|
Created:
4 years, 9 months ago by Gang Wu Modified:
4 years, 9 months ago CC:
chromium-reviews, tim+watch_chromium.org, maxbogue+watch_chromium.org, plaree+watch_chromium.org, zea+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet ModelTypeService create SharedModelTypeProcessor instead
of ModelTypeController.
Only change the place where to create, still need to figure
out where to trigger.
BUG=547087
Committed: https://crrev.com/247ad02364179cbc1b117bfcb67d086a5456235a
Cr-Commit-Position: refs/heads/master@{#383357}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : #Patch Set 3 : #
Total comments: 7
Patch Set 4 : #
Total comments: 28
Patch Set 5 : #
Total comments: 48
Patch Set 6 : update base on Max and Sky's comment #
Total comments: 29
Patch Set 7 : Sky's review #
Total comments: 18
Patch Set 8 : update base on recent review #
Total comments: 4
Patch Set 9 : nit #Messages
Total messages: 47 (22 generated)
Patchset #4 (id:60001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Description was changed from ========== first checkin BUG= ========== to ========== Let ModelTypeService create SharedModelTypeProcessor instead of ModelTypeController. BUG=547087 ==========
Description was changed from ========== Let ModelTypeService create SharedModelTypeProcessor instead of ModelTypeController. BUG=547087 ========== to ========== Let ModelTypeService create SharedModelTypeProcessor instead of ModelTypeController. Only change the place where to create, still need to figure out where to trigger. BUG=547087 ==========
gangwu@chromium.org changed reviewers: + maxbogue@chromium.org, pavely@chromium.org, skym@chromium.org
Patchset #1 (id:40001) has been deleted
PTAL
Creation should be triggered in 1) void DeviceInfoService::OnReadAllMetadata( 2) Somewhere in the logic Pavel is adding https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service_unittest.cc:211: void SetProcessorAndPump() { I don't think we need this method anymore. Originally we assumed the processor creation was being driving from something outside the service, and that's essentially what this method did. But now, the service drives that, so there's not reason to call this method. https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.cc:58: base::Unretained(type_processor()), Danger! https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.cc:154: base::Unretained(type_processor()))); I think it's important we have a weak ptr to verify things are still alive. https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.cc:220: if (!model_type_service.get()) { This is only safe on the model thread. https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... File components/sync_driver/non_blocking_data_type_controller.h (right): https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.h:65: syncer_v2::SharedModelTypeProcessor* type_processor() const; I'm not convinced this is a safe change. https://codereview.chromium.org/1763953002/diff/80001/sync/api/model_type_ser... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/80001/sync/api/model_type_ser... sync/api/model_type_service.h:82: virtual scoped_ptr<SharedModelTypeProcessor> CreateSharedModelTypeProcessor( Overriding virtual methods for so tests can inject dependencies is definitely not my favorite. Have you tried passing something into the constructor?
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Patchset #2 (id:140001) has been deleted
Patchset #2 (id:160001) has been deleted
updated https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service_unittest.cc:211: void SetProcessorAndPump() { On 2016/03/04 22:12:28, skym wrote: > I don't think we need this method anymore. Originally we assumed the processor > creation was being driving from something outside the service, and that's > essentially what this method did. But now, the service drives that, so there's > not reason to call this method. Will update where to trigger initial processor in next CL since it will require some works. https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.cc:58: base::Unretained(type_processor()), On 2016/03/04 22:12:28, skym wrote: > Danger! Done. https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.cc:154: base::Unretained(type_processor()))); On 2016/03/04 22:12:28, skym wrote: > I think it's important we have a weak ptr to verify things are still alive. Done. https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.cc:220: if (!model_type_service.get()) { On 2016/03/04 22:12:28, skym wrote: > This is only safe on the model thread. Done. https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... File components/sync_driver/non_blocking_data_type_controller.h (right): https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/... components/sync_driver/non_blocking_data_type_controller.h:65: syncer_v2::SharedModelTypeProcessor* type_processor() const; On 2016/03/04 22:12:28, skym wrote: > I'm not convinced this is a safe change. Done. https://codereview.chromium.org/1763953002/diff/80001/sync/api/model_type_ser... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/80001/sync/api/model_type_ser... sync/api/model_type_service.h:82: virtual scoped_ptr<SharedModelTypeProcessor> CreateSharedModelTypeProcessor( On 2016/03/04 22:12:28, skym wrote: > Overriding virtual methods for so tests can inject dependencies is definitely > not my favorite. Have you tried passing something into the constructor? Done.
Patchset #3 (id:200001) has been deleted
Patchset #3 (id:220001) has been deleted
updated base on discussion
ping
I think you missed the very end of our discussion with this CL. I believe we decided that since the service will often need to create the processor before the DTC exists, we would just do the cast in the DTC for now. https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:160: void NonBlockingDataTypeController::StopOnModelThread() { Why did you switch to having this extra function instead of binding DisconnectSync directly? https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:243: model_type_service->InitializeProcessor(base::Bind( As we discussed at the very end of the meeting, this function should simply get cast the return value of ModelTypeService::OnSyncStarting(). https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:260: NonBlockingDataTypeController::CreateSharedModelTypeProcessor( This shouldn't be in the DTC. It needs to be passed into the constructor of the service. https://codereview.chromium.org/1763953002/diff/240001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/240001/sync/api/model_type_se... sync/api/model_type_service.h:90: ModelTypeChangeProcessor* InitializeProcessor( The service might need to create the processor before the DTC exists. It needs to receive the factory function in the constructor, and this function should have the signature: ModelTypeChangeProcessor* OnSyncStarting();
Patchset #4 (id:260001) has been deleted
updated. https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:160: void NonBlockingDataTypeController::StopOnModelThread() { On 2016/03/15 17:37:33, maxbogue wrote: > Why did you switch to having this extra function instead of binding > DisconnectSync directly? Doing this because my previous patch, change it back. https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:243: model_type_service->InitializeProcessor(base::Bind( On 2016/03/15 17:37:33, maxbogue wrote: > As we discussed at the very end of the meeting, this function should simply get > cast the return value of ModelTypeService::OnSyncStarting(). Done. https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:260: NonBlockingDataTypeController::CreateSharedModelTypeProcessor( On 2016/03/15 17:37:33, maxbogue wrote: > This shouldn't be in the DTC. It needs to be passed into the constructor of the > service. Done.
https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/device_info_service.h (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/device_info_service.h:74: syncer::ModelType type() const override; You added this to the DeviceInfoTracker methods, while it should be with the ModelTypeService methods, right? https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:81: syncer_v2::ModelTypeChangeProcessor* model_type_change_processor = This order of operations makes me really uneasy. If the processor already exists, it's going to invoke our callback in task, and that will happen before we get around to assigning this member field. Looks like our current logic is safe but we're kind of setting a trap for future selves. Is there a reason we need the processor synchronously here? Does it make more sense to push that into the callback and remove the change_processor() accessor? What do others think? I'm not sure what's better. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:87: type_processor_ = static_cast<syncer_v2::SharedModelTypeProcessor*>( What happens if this cast fails? Should there be another check? https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.h (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.h:19: class ModelTypeChangeProcessor; Looks like you didn't need these. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.h:107: // The function will do the work on model thread for function LoadModels(). What does this mean? What is 'function LoadModels()'? https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.cc:38: // TODO(gangwu): should not cast here, find a better way to call. Ooooh yeah I didn't realize we were going to have to cast for this to work. Hmm.. Does it make sense to move this method to the left hand side processor interface? https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:35: syncer::ModelType, Can you curry the model type before it gets to the service? https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:37: SharedProcessorFactory; I like that you named this callback XxFactory. https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:90: virtual syncer::ModelType type() const = 0; I'm not convinced you need this. https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:93: ModelTypeChangeProcessor* change_processor(); Why remove const? https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:97: void OnSyncStarting(StartCallback callback); Typically we pass around callbacks as const refs. https://codereview.chromium.org/1763953002/diff/280001/sync/internal_api/publ... File sync/internal_api/public/shared_model_type_processor.h (right): https://codereview.chromium.org/1763953002/diff/280001/sync/internal_api/publ... sync/internal_api/public/shared_model_type_processor.h:45: void OnSyncStarting(ModelTypeService::StartCallback callback); While you're in here ;) can you change this to be (const ModelTypeService::StartCallback& callback). Not sure, but we might be copying it multiple times unnecessarily.
Patchset #5 (id:300001) has been deleted
updated base on sky's comments. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:81: syncer_v2::ModelTypeChangeProcessor* model_type_change_processor = On 2016/03/22 15:56:26, skym wrote: > This order of operations makes me really uneasy. If the processor already > exists, it's going to invoke our callback in task, and that will happen before > we get around to assigning this member field. Looks like our current logic is > safe but we're kind of setting a trap for future selves. > > Is there a reason we need the processor synchronously here? Does it make more > sense to push that into the callback and remove the change_processor() accessor? > What do others think? I'm not sure what's better. Yes, I agree. This order is not good. And also will cause ModelTypeService to cast processor from ModelTypeChangeProcessor to SharedModelTypeProcessor(see model_type_service.cc).I think maybe we can keep previous order. 1. controller ask service for processor, if not exist, please create one. 2. controller call OnSyncStarting. what others think? https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:87: type_processor_ = static_cast<syncer_v2::SharedModelTypeProcessor*>( On 2016/03/22 15:56:26, skym wrote: > What happens if this cast fails? Should there be another check? I am not familiar with static_cast, but I think static_cast is not necessary to check. maybe change to dynamic_cast? since dynamic_cast will return null if failed to cast. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.h (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.h:19: class ModelTypeChangeProcessor; On 2016/03/22 15:56:26, skym wrote: > Looks like you didn't need these. Done. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.h:107: // The function will do the work on model thread for function LoadModels(). On 2016/03/22 15:56:26, skym wrote: > What does this mean? What is 'function LoadModels()'? better? https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.cc:38: // TODO(gangwu): should not cast here, find a better way to call. On 2016/03/22 15:56:26, skym wrote: > Ooooh yeah I didn't realize we were going to have to cast for this to work. > Hmm.. Does it make sense to move this method to the left hand side processor > interface? put this method into ModelTypeChangeProcessor is weird as well, since ModelTypeChangeProcessor is to inform sync of local changes, and the callback is to tell controller/service that processor is started. https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:37: SharedProcessorFactory; On 2016/03/22 15:56:26, skym wrote: > I like that you named this callback XxFactory. Done. https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:93: ModelTypeChangeProcessor* change_processor(); On 2016/03/22 15:56:26, skym wrote: > Why remove const? Done. https://codereview.chromium.org/1763953002/diff/280001/sync/api/model_type_se... sync/api/model_type_service.h:97: void OnSyncStarting(StartCallback callback); On 2016/03/22 15:56:26, skym wrote: > Typically we pass around callbacks as const refs. Done. https://codereview.chromium.org/1763953002/diff/280001/sync/internal_api/publ... File sync/internal_api/public/shared_model_type_processor.h (right): https://codereview.chromium.org/1763953002/diff/280001/sync/internal_api/publ... sync/internal_api/public/shared_model_type_processor.h:45: void OnSyncStarting(ModelTypeService::StartCallback callback); On 2016/03/22 15:56:26, skym wrote: > While you're in here ;) can you change this to be (const > ModelTypeService::StartCallback& callback). Not sure, but we might be copying it > multiple times unnecessarily. Done. https://codereview.chromium.org/1763953002/diff/280001/sync/internal_api/shar... File sync/internal_api/shared_model_type_processor_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/280001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:365: size_t ProcessorEntityCount() { will add const back https://codereview.chromium.org/1763953002/diff/280001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:396: SharedModelTypeProcessor* type_processor() { will add const back
https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:81: syncer_v2::ModelTypeChangeProcessor* model_type_change_processor = On 2016/03/22 17:47:50, Gang Wu wrote: > On 2016/03/22 15:56:26, skym wrote: > > This order of operations makes me really uneasy. If the processor already > > exists, it's going to invoke our callback in task, and that will happen before > > we get around to assigning this member field. Looks like our current logic is > > safe but we're kind of setting a trap for future selves. > > > > Is there a reason we need the processor synchronously here? Does it make more > > sense to push that into the callback and remove the change_processor() > accessor? > > What do others think? I'm not sure what's better. > > Yes, I agree. > This order is not good. And also will cause ModelTypeService to cast processor > from ModelTypeChangeProcessor to SharedModelTypeProcessor(see > model_type_service.cc).I think maybe we can keep previous order. > 1. controller ask service for processor, if not exist, please create one. > 2. controller call OnSyncStarting. > what others think? Your approach at first makes sense, all the right pieces of data go to the right places. But if you take a step back, it becomes a little awkward. I think the idea we had last week during our meeting was that this should be a signal to the service that we are starting sync, and it should be loading the metadata right now if it hasn't already. Right now we have the OnChangeProcessorSet method that kind of performs this role, but it's really ugly. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:87: type_processor_ = static_cast<syncer_v2::SharedModelTypeProcessor*>( On 2016/03/22 17:47:50, Gang Wu wrote: > On 2016/03/22 15:56:26, skym wrote: > > What happens if this cast fails? Should there be another check? > > I am not familiar with static_cast, but I think static_cast is not necessary to > check. > maybe change to dynamic_cast? since dynamic_cast will return null if failed to > cast. Yeah, after doing some reading it sounds like dynamic_cast is the right thing to do here. We shouldn't static_casting from base to child classes. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.h (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.h:19: class ModelTypeChangeProcessor; On 2016/03/22 17:47:51, Gang Wu wrote: > On 2016/03/22 15:56:26, skym wrote: > > Looks like you didn't need these. > > Done. You don't need ModelTypeService either, I think.
Hey Gang, sorry for the wall of comments but I had a lot of concerns. If anything is unclear feel free to ping me on chat to clarify. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service.cc:46: const SharedProcessorFactory& shared_processor_factory) change_processor_factory https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/device_info_service.h (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service.h:50: const SharedProcessorFactory& shared_processor_factory); change_processor_factory https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service.h:74: syncer::ModelType type() const override; This is in the wrong section; it should be the ModelTypeService impl section. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:120: : public syncer_v2::SharedModelTypeProcessor { Why is this now an actual SharedModelTypeProcessor? https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:156: class FakeDeviceInfoService : public DeviceInfoService { You shouldn't need this class. It's not adding any useful information... https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:78: model_type_service->OnSyncStarting( This call should return a ModelTypeChangeProcessor* directly, and that is what should be static_cast and stored. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:83: if (model_type_change_processor == NULL) { Just DCHECK. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/non_ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/non_ui_model_type_controller_unittest.cc:187: service_->CreateProcessorForTest(); You're using CreateProcessorForTest to sidestep the fact that you just fundamentally changed functionality of this class. That function should be protected in FakeModelTypeService, and these tests should be updated to reflect the new flow of how the processor is made. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:21: ModelTypeChangeProcessor* ModelTypeService::create_change_processor() { This function is doing nontrivial logic, which means it should be named CreateChangeProcessor. I also think you can just return void here since any callers should be able to access the processor via change_processor(). It's cleaner to only have one way to access the pointer. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:22: if (!change_processor_.get()) { I think you should just either just DCHECK(!change_processor_) and make callers check change_processor() or name the function GetOrCreateChangeProcessor. I would prefer the former, as it makes it more explicit when the creation is happening. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:25: if (!change_processor_.get()) { Again, better to just DCHECK(change_processor_). https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:39: static_cast<SharedModelTypeProcessor*>(create_change_processor()) OnSyncStarting should just be moved into the ModelTypeChangeProcessor interface. Please either do so or have the TODO here say that. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.h:37: SharedProcessorFactory; Please call this ChangeProcessorFactory. ModelTypeChangeProcessor is the interface. SharedModelTypeProcessor is the implementation that this class does not know about, so things shouldn't be named for it. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.h:97: void OnSyncStarting(const StartCallback& callback); I think you should be returning ModelTypeChangeProcessor* here. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/publ... File sync/internal_api/public/test/fake_model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/publ... sync/internal_api/public/test/fake_model_type_service.h:48: ModelTypeChangeProcessor* CreateProcessorForTest(); This should be protected. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... File sync/internal_api/shared_model_type_processor_unittest.cc (left): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:359: size_t ProcessorEntityCount() const { Why removing const? https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:390: SharedModelTypeProcessor* type_processor() const { Keep const? https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... File sync/internal_api/shared_model_type_processor_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:151: void CreateProcessor() { I think you should be able to remove this and just call CreateChangeProcessor() everywhere it was used before. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:156: scoped_ptr<ModelTypeChangeProcessor> CreateSharedModelTypeProcessor( This should be removed. I'm pretty sure it's not doing anything right now. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... File sync/internal_api/test/fake_model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:8: #include "sync/internal_api/public/shared_model_type_processor.h" You should not be including this real class in a fake. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:14: base::Bind(&FakeModelTypeService::CreateSharedModelTypeProcessor)) {} This should bind a private function (CreateProcessorForTestWrapper) that calls a protected virtual function (CreateProcessorForTest) that can be overwritten but returns nullptr by default. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:51: return create_change_processor(); Just return nullptr by default here.
https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:22: if (!change_processor_.get()) { On 2016/03/22 21:40:12, maxbogue wrote: > I think you should just either just DCHECK(!change_processor_) and make callers > check change_processor() or name the function GetOrCreateChangeProcessor. I > would prefer the former, as it makes it more explicit when the creation is > happening. +1 to GetOrCreateChangeProcessor. Checking change_procesor() is clunky, and every service would have to do this. Also, you might be able to make change_processor() protected now that the controller should only call the GetOrCreateChangeProcessor() method. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... File sync/internal_api/test/fake_model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:14: base::Bind(&FakeModelTypeService::CreateSharedModelTypeProcessor)) {} On 2016/03/22 21:40:12, maxbogue wrote: > This should bind a private function (CreateProcessorForTestWrapper) that calls a > protected virtual function (CreateProcessorForTest) that can be overwritten but > returns nullptr by default. Could also set a default argument for the factory method so anyone that cares can specify it in the constructor call. Not 100% sure this works with binding.
https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... File sync/internal_api/test/fake_model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:14: base::Bind(&FakeModelTypeService::CreateSharedModelTypeProcessor)) {} On 2016/03/22 22:18:18, skym wrote: > On 2016/03/22 21:40:12, maxbogue wrote: > > This should bind a private function (CreateProcessorForTestWrapper) that calls > a > > protected virtual function (CreateProcessorForTest) that can be overwritten > but > > returns nullptr by default. > > Could also set a default argument for the factory method so anyone that cares > can specify it in the constructor call. Not 100% sure this works with binding. Max pointed out we shouldn't do this, he found https://google.github.io/styleguide/cppguide.html#Default_Arguments . So either virtual method or overloaded constructor I guess are the choices.
Patchset #6 (id:340001) has been deleted
Patchset #7 (id:380001) has been deleted
Patchset #6 (id:360001) has been deleted
update, did not remove OnChangeProcessorSet, will remove in next CL. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:87: type_processor_ = static_cast<syncer_v2::SharedModelTypeProcessor*>( On 2016/03/22 19:02:06, skym wrote: > On 2016/03/22 17:47:50, Gang Wu wrote: > > On 2016/03/22 15:56:26, skym wrote: > > > What happens if this cast fails? Should there be another check? > > > > I am not familiar with static_cast, but I think static_cast is not necessary > to > > check. > > maybe change to dynamic_cast? since dynamic_cast will return null if failed to > > cast. > > Yeah, after doing some reading it sounds like dynamic_cast is the right thing to > do here. We shouldn't static_casting from base to child classes. After I use dynamic_cast, I receive a compiler error. then realize if we want to use dynamic_cast, we need to enable rtti for compiler. And after checking Google C++ style guide, it does not recommend to use dynamic_cast for non-unittest. and since we are sure to always create SharedModelTypeProcessor, I think static_cast is good to use her. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.h (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.h:19: class ModelTypeChangeProcessor; On 2016/03/22 19:02:06, skym wrote: > On 2016/03/22 17:47:51, Gang Wu wrote: > > On 2016/03/22 15:56:26, skym wrote: > > > Looks like you didn't need these. > > > > Done. > > You don't need ModelTypeService either, I think. Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service.cc:46: const SharedProcessorFactory& shared_processor_factory) On 2016/03/22 21:40:11, maxbogue wrote: > change_processor_factory Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/device_info_service.h (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service.h:50: const SharedProcessorFactory& shared_processor_factory); On 2016/03/22 21:40:11, maxbogue wrote: > change_processor_factory Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service.h:74: syncer::ModelType type() const override; On 2016/03/22 21:40:12, maxbogue wrote: > This is in the wrong section; it should be the ModelTypeService impl section. Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:120: : public syncer_v2::SharedModelTypeProcessor { On 2016/03/22 21:40:12, maxbogue wrote: > Why is this now an actual SharedModelTypeProcessor? Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:156: class FakeDeviceInfoService : public DeviceInfoService { On 2016/03/22 21:40:12, maxbogue wrote: > You shouldn't need this class. It's not adding any useful information... Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:78: model_type_service->OnSyncStarting( On 2016/03/22 21:40:12, maxbogue wrote: > This call should return a ModelTypeChangeProcessor* directly, and that is what > should be static_cast and stored. Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:83: if (model_type_change_processor == NULL) { On 2016/03/22 21:40:12, maxbogue wrote: > Just DCHECK. Done. https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... File components/sync_driver/non_ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/320001/components/sync_driver... components/sync_driver/non_ui_model_type_controller_unittest.cc:187: service_->CreateProcessorForTest(); On 2016/03/22 21:40:12, maxbogue wrote: > You're using CreateProcessorForTest to sidestep the fact that you just > fundamentally changed functionality of this class. That function should be > protected in FakeModelTypeService, and these tests should be updated to reflect > the new flow of how the processor is made. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:21: ModelTypeChangeProcessor* ModelTypeService::create_change_processor() { On 2016/03/22 21:40:12, maxbogue wrote: > This function is doing nontrivial logic, which means it should be named > CreateChangeProcessor. I also think you can just return void here since any > callers should be able to access the processor via change_processor(). It's > cleaner to only have one way to access the pointer. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:22: if (!change_processor_.get()) { On 2016/03/22 22:18:17, skym wrote: > On 2016/03/22 21:40:12, maxbogue wrote: > > I think you should just either just DCHECK(!change_processor_) and make > callers > > check change_processor() or name the function GetOrCreateChangeProcessor. I > > would prefer the former, as it makes it more explicit when the creation is > > happening. > > +1 to GetOrCreateChangeProcessor. Checking change_procesor() is clunky, and > every service would have to do this. Also, you might be able to make > change_processor() protected now that the controller should only call the > GetOrCreateChangeProcessor() method. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:22: if (!change_processor_.get()) { On 2016/03/22 21:40:12, maxbogue wrote: > I think you should just either just DCHECK(!change_processor_) and make callers > check change_processor() or name the function GetOrCreateChangeProcessor. I > would prefer the former, as it makes it more explicit when the creation is > happening. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:25: if (!change_processor_.get()) { On 2016/03/22 21:40:12, maxbogue wrote: > Again, better to just DCHECK(change_processor_). Done. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.cc:39: static_cast<SharedModelTypeProcessor*>(create_change_processor()) On 2016/03/22 21:40:12, maxbogue wrote: > OnSyncStarting should just be moved into the ModelTypeChangeProcessor interface. > Please either do so or have the TODO here say that. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.h:37: SharedProcessorFactory; On 2016/03/22 21:40:12, maxbogue wrote: > Please call this ChangeProcessorFactory. ModelTypeChangeProcessor is the > interface. SharedModelTypeProcessor is the implementation that this class does > not know about, so things shouldn't be named for it. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_se... sync/api/model_type_service.h:97: void OnSyncStarting(const StartCallback& callback); On 2016/03/22 21:40:12, maxbogue wrote: > I think you should be returning ModelTypeChangeProcessor* here. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/publ... File sync/internal_api/public/test/fake_model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/publ... sync/internal_api/public/test/fake_model_type_service.h:48: ModelTypeChangeProcessor* CreateProcessorForTest(); On 2016/03/22 21:40:12, maxbogue wrote: > This should be protected. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... File sync/internal_api/shared_model_type_processor_unittest.cc (left): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:359: size_t ProcessorEntityCount() const { On 2016/03/22 21:40:12, maxbogue wrote: > Why removing const? Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:390: SharedModelTypeProcessor* type_processor() const { On 2016/03/22 21:40:12, maxbogue wrote: > Keep const? Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... File sync/internal_api/shared_model_type_processor_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:151: void CreateProcessor() { On 2016/03/22 21:40:12, maxbogue wrote: > I think you should be able to remove this and just call CreateChangeProcessor() > everywhere it was used before. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/shar... sync/internal_api/shared_model_type_processor_unittest.cc:156: scoped_ptr<ModelTypeChangeProcessor> CreateSharedModelTypeProcessor( On 2016/03/22 21:40:12, maxbogue wrote: > This should be removed. I'm pretty sure it's not doing anything right now. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... File sync/internal_api/test/fake_model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:8: #include "sync/internal_api/public/shared_model_type_processor.h" On 2016/03/22 21:40:12, maxbogue wrote: > You should not be including this real class in a fake. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:14: base::Bind(&FakeModelTypeService::CreateSharedModelTypeProcessor)) {} On 2016/03/22 21:40:12, maxbogue wrote: > This should bind a private function (CreateProcessorForTestWrapper) that calls a > protected virtual function (CreateProcessorForTest) that can be overwritten but > returns nullptr by default. Done. https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test... sync/internal_api/test/fake_model_type_service.cc:51: return create_change_processor(); On 2016/03/22 21:40:12, maxbogue wrote: > Just return nullptr by default here. Done.
I also hate OnChangeProcessorSet! I'm curious how you plan to get rid of it though. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:375: if (metadata_records->size() > 0) { Should also create a proc if the global_metadata isn't empty. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:376: GetOrCreateChangeProcessor(); This GetOrCreateChangeProcessor() method is turning out to be more awkward than I thought it was going to be, I may have been wrong when I thought this was the right way to do things. Lets leave it as is though until we remove the processor from the controller, then we can clean it up. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:378: if (!change_processor()) { This race condition doesn't exist anymore. Can remove this block. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:429: if (has_data_loaded_) { This is significantly wrong now. We're going to double load metadata and the proc is going to fail a DCHECK. I'm okay with you just adding a TODO in here and I'll fix this right after you land this. It's very tightly coupled to changes I'm in the process of making. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:435: syncer::ModelType DeviceInfoService::type() const { Another way to do this. You could move this into the base class and have each impl pass in their type upon construction. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:78: syncer_v2::ModelTypeChangeProcessor* model_type_change_processor = Can you throw in a TODO explaining how we want to stop holding onto the proc in the controller? https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/non_ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/non_ui_model_type_controller_unittest.cc:148: class FakeNonUIModelTypeService : public syncer_v2::FakeModelTypeService { This is a lot of boilerplate copy/pasted from ui_model_type_controller_unittest.cc :( https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/ui_model_type_controller_unittest.cc:131: class FakeUIModelTypeService : public syncer_v2::FakeModelTypeService { You wouldn't need this class if you used an overloaded constructor instead of overriding a virtual method ;) https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:7: #include "sync/internal_api/public/shared_model_type_processor.h" Replace this with correct include. https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:22: if (!change_processor_.get()) { I think this .get() is unnecessary. https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:24: change_processor_factory_.Run(type(), this).release()); Is there a reason you're not using std::swap anymore? It seems more elegant to me than this reset/release combo. https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:25: DCHECK(change_processor_.get()); I think this .get() in unnecessary. https://codereview.chromium.org/1763953002/diff/400001/sync/internal_api/publ... File sync/internal_api/public/test/fake_model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/400001/sync/internal_api/publ... sync/internal_api/public/test/fake_model_type_service.h:50: virtual ModelTypeChangeProcessor* CreateProcessorForTest( I still think overloading the constructor is cleaner than overriding virtual methods.
Did not use overload contructor for FakeModelTypeService since it will involve a lot of work to update ui/non_ui_model_type_controller_unittest, if do not like current solution, I can change in another CL. For removing OnChangeProcessorSet, what I mean is just change/rename to load_metadata, not remove it completely. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:375: if (metadata_records->size() > 0) { On 2016/03/24 16:08:52, skym wrote: > Should also create a proc if the global_metadata isn't empty. Done. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:376: GetOrCreateChangeProcessor(); On 2016/03/24 16:08:51, skym wrote: > This GetOrCreateChangeProcessor() method is turning out to be more awkward than > I thought it was going to be, I may have been wrong when I thought this was the > right way to do things. Lets leave it as is though until we remove the processor > from the controller, then we can clean it up. Done. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:378: if (!change_processor()) { On 2016/03/24 16:08:51, skym wrote: > This race condition doesn't exist anymore. Can remove this block. I think we should keep it if metadata_records->size() == 0 && global_metadata.size() == 0 https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:429: if (has_data_loaded_) { On 2016/03/24 16:08:52, skym wrote: > This is significantly wrong now. We're going to double load metadata and the > proc is going to fail a DCHECK. I'm okay with you just adding a TODO in here and > I'll fix this right after you land this. It's very tightly coupled to changes > I'm in the process of making. Done. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/non_blocking_data_type_controller.cc:78: syncer_v2::ModelTypeChangeProcessor* model_type_change_processor = On 2016/03/24 16:08:52, skym wrote: > Can you throw in a TODO explaining how we want to stop holding onto the proc in > the controller? Done. https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/non_ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/non_ui_model_type_controller_unittest.cc:148: class FakeNonUIModelTypeService : public syncer_v2::FakeModelTypeService { On 2016/03/24 16:08:52, skym wrote: > This is a lot of boilerplate copy/pasted from > ui_model_type_controller_unittest.cc :( move duplicate code into FakeModelTypeService https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/ui_model_type_controller_unittest.cc:131: class FakeUIModelTypeService : public syncer_v2::FakeModelTypeService { On 2016/03/24 16:08:52, skym wrote: > You wouldn't need this class if you used an overloaded constructor instead of > overriding a virtual method ;) This test uses direct access to processor for loading metadata. If we change to use overloaded constructor, then for triggering creating processor during loading metadata, this test need to create a ModelTypeStore to do it. Since this CL is large enough, I can do it in another CL. https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:7: #include "sync/internal_api/public/shared_model_type_processor.h" On 2016/03/24 16:08:52, skym wrote: > Replace this with correct include. Done. https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:22: if (!change_processor_.get()) { On 2016/03/24 16:08:52, skym wrote: > I think this .get() is unnecessary. Done. https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:24: change_processor_factory_.Run(type(), this).release()); On 2016/03/24 16:08:52, skym wrote: > Is there a reason you're not using std::swap anymore? It seems more elegant to > me than this reset/release combo. Done. https://codereview.chromium.org/1763953002/diff/400001/sync/api/model_type_se... sync/api/model_type_service.cc:25: DCHECK(change_processor_.get()); On 2016/03/24 16:08:52, skym wrote: > I think this .get() in unnecessary. Done. https://codereview.chromium.org/1763953002/diff/400001/sync/internal_api/publ... File sync/internal_api/public/test/fake_model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/400001/sync/internal_api/publ... sync/internal_api/public/test/fake_model_type_service.h:50: virtual ModelTypeChangeProcessor* CreateProcessorForTest( On 2016/03/24 16:08:52, skym wrote: > I still think overloading the constructor is cleaner than overriding virtual > methods. I can do it in another CL.
lgtm https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:378: if (!change_processor()) { On 2016/03/24 22:18:11, Gang Wu wrote: > On 2016/03/24 16:08:51, skym wrote: > > This race condition doesn't exist anymore. Can remove this block. > > I think we should keep it if metadata_records->size() == 0 && > global_metadata.size() == 0 Okay, you're right. Ugh, the correct behavior here is really subtle. I don't like that we've got ourselves into this scenario. Not sure how to make it better though. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service.cc:429: // TODO(skym): fix the double load metadata issue nit: Comments should look like sentences with capital first letter and period at the end. :P https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:22: #include "sync/internal_api/public/shared_model_type_processor.h" remove. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:169: scoped_ptr<ModelTypeChangeProcessor> CreateSharedModelTypeProcessor( Name of method contains "Shared". https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:211: EXPECT_EQ(processor_, service_->GetOrCreateChangeProcessor()); Can you also put a TODO(skym): Shouldn't need to directly force processor creation anymore. https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... sync/api/model_type_service.cc:23: change_processor_ = change_processor_factory_.Run(type(), this); Cool! I didn't know this worked. https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... sync/api/model_type_service.h:24: class SharedModelTypeProcessor; Remove.
https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:435: syncer::ModelType DeviceInfoService::type() const { On 2016/03/24 16:08:51, skym wrote: > Another way to do this. You could move this into the base class and have each > impl pass in their type upon construction. +1, I like the idea of just tossing the type in the base constructor. https://codereview.chromium.org/1763953002/diff/400001/sync/internal_api/publ... File sync/internal_api/public/test/fake_model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/400001/sync/internal_api/publ... sync/internal_api/public/test/fake_model_type_service.h:50: virtual ModelTypeChangeProcessor* CreateProcessorForTest( On 2016/03/24 16:08:52, skym wrote: > I still think overloading the constructor is cleaner than overriding virtual > methods. I'm pretty please with this way because you don't need any binds in subclasses, but either is fine with me. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:210: void SetProcessorAndPump() { Could you rename this to CreateProcessorAndPump? https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/non_ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/non_ui_model_type_controller_unittest.cc:186: type_processor_ = This flow is super weird, but it's fine for now. Could to add a TODO with http://crbug.com/543407 linked in it to move the processor stuff out? https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/ui_model_type_controller_unittest.cc:146: type_processor_ = This flow is super weird, but it's fine for now. Could to add a TODO with http://crbug.com/543407 linked in it to move the processor stuff out?
update https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver... components/sync_driver/device_info_service.cc:435: syncer::ModelType DeviceInfoService::type() const { On 2016/03/24 23:28:47, maxbogue wrote: > On 2016/03/24 16:08:51, skym wrote: > > Another way to do this. You could move this into the base class and have each > > impl pass in their type upon construction. > > +1, I like the idea of just tossing the type in the base constructor. Done. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service.cc:429: // TODO(skym): fix the double load metadata issue On 2016/03/24 22:51:37, skym wrote: > nit: Comments should look like sentences with capital first letter and period at > the end. :P Done. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:22: #include "sync/internal_api/public/shared_model_type_processor.h" On 2016/03/24 22:51:37, skym wrote: > remove. Done. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:169: scoped_ptr<ModelTypeChangeProcessor> CreateSharedModelTypeProcessor( On 2016/03/24 22:51:37, skym wrote: > Name of method contains "Shared". Done. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:210: void SetProcessorAndPump() { On 2016/03/24 23:28:47, maxbogue wrote: > Could you rename this to CreateProcessorAndPump? Done. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/device_info_service_unittest.cc:211: EXPECT_EQ(processor_, service_->GetOrCreateChangeProcessor()); On 2016/03/24 22:51:37, skym wrote: > Can you also put a TODO(skym): Shouldn't need to directly force processor > creation anymore. Done. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/non_ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/non_ui_model_type_controller_unittest.cc:186: type_processor_ = On 2016/03/24 23:28:47, maxbogue wrote: > This flow is super weird, but it's fine for now. Could to add a TODO with > http://crbug.com/543407 linked in it to move the processor stuff out? Done. https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... File components/sync_driver/ui_model_type_controller_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/420001/components/sync_driver... components/sync_driver/ui_model_type_controller_unittest.cc:146: type_processor_ = On 2016/03/24 23:28:47, maxbogue wrote: > This flow is super weird, but it's fine for now. Could to add a TODO with > http://crbug.com/543407 linked in it to move the processor stuff out? Done. https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... sync/api/model_type_service.cc:23: change_processor_ = change_processor_factory_.Run(type(), this); On 2016/03/24 22:51:37, skym wrote: > Cool! I didn't know this worked. Done. https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/420001/sync/api/model_type_se... sync/api/model_type_service.h:24: class SharedModelTypeProcessor; On 2016/03/24 22:51:37, skym wrote: > Remove. Done.
lgtm! Thanks for your patience with this Gang! https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver... components/sync_driver/device_info_service.cc:375: if (metadata_records->size() > 0 || global_metadata.size() > 0) { nit: !global_metadata.empty() instead of global_metadata.size() > 0? https://codereview.chromium.org/1763953002/diff/440001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/440001/sync/api/model_type_se... sync/api/model_type_service.h:88: syncer::ModelType type() const; I think you can just remove this method entirely now; is it needed somewhere? Or maybe make it protected?
updated. https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver... components/sync_driver/device_info_service.cc:375: if (metadata_records->size() > 0 || global_metadata.size() > 0) { On 2016/03/25 17:32:41, maxbogue wrote: > nit: !global_metadata.empty() instead of global_metadata.size() > 0? Done. https://codereview.chromium.org/1763953002/diff/440001/sync/api/model_type_se... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1763953002/diff/440001/sync/api/model_type_se... sync/api/model_type_service.h:88: syncer::ModelType type() const; On 2016/03/25 17:32:41, maxbogue wrote: > I think you can just remove this method entirely now; is it needed somewhere? Or > maybe make it protected? Done.
lgtm
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org, maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/1763953002/#ps460001 (title: "nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1763953002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1763953002/460001
Message was sent while issue was closed.
Description was changed from ========== Let ModelTypeService create SharedModelTypeProcessor instead of ModelTypeController. Only change the place where to create, still need to figure out where to trigger. BUG=547087 ========== to ========== Let ModelTypeService create SharedModelTypeProcessor instead of ModelTypeController. Only change the place where to create, still need to figure out where to trigger. BUG=547087 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== Let ModelTypeService create SharedModelTypeProcessor instead of ModelTypeController. Only change the place where to create, still need to figure out where to trigger. BUG=547087 ========== to ========== Let ModelTypeService create SharedModelTypeProcessor instead of ModelTypeController. Only change the place where to create, still need to figure out where to trigger. BUG=547087 Committed: https://crrev.com/247ad02364179cbc1b117bfcb67d086a5456235a Cr-Commit-Position: refs/heads/master@{#383357} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/247ad02364179cbc1b117bfcb67d086a5456235a Cr-Commit-Position: refs/heads/master@{#383357} |
