Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(466)

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
Reviewers:
maxbogue, skym, pavely
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.

Description

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}

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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+186 lines, -93 lines) Patch
M components/sync_driver/device_info_model_type_controller.cc View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M components/sync_driver/device_info_service.h View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M components/sync_driver/device_info_service.cc View 1 2 3 4 5 6 7 8 3 chunks +9 lines, -3 lines 0 comments Download
M components/sync_driver/device_info_service_unittest.cc View 1 2 3 4 5 6 7 10 chunks +22 lines, -11 lines 0 comments Download
M components/sync_driver/non_blocking_data_type_controller.h View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -4 lines 0 comments Download
M components/sync_driver/non_blocking_data_type_controller.cc View 1 2 3 4 5 6 7 8 4 chunks +24 lines, -32 lines 0 comments Download
M components/sync_driver/non_ui_model_type_controller_unittest.cc View 1 2 3 4 5 6 7 3 chunks +4 lines, -4 lines 0 comments Download
M components/sync_driver/ui_model_type_controller_unittest.cc View 1 2 3 4 5 6 7 2 chunks +4 lines, -5 lines 0 comments Download
M sync/api/model_type_change_processor.h View 1 2 3 4 5 3 chunks +10 lines, -0 lines 0 comments Download
M sync/api/model_type_service.h View 1 2 3 4 5 6 7 8 3 chunks +20 lines, -5 lines 0 comments Download
M sync/api/model_type_service.cc View 1 2 3 4 5 6 7 2 chunks +25 lines, -6 lines 0 comments Download
M sync/internal_api/public/shared_model_type_processor.h View 1 2 3 4 5 2 chunks +1 line, -9 lines 0 comments Download
M sync/internal_api/public/test/fake_model_type_service.h View 1 2 3 4 5 6 7 2 chunks +18 lines, -0 lines 0 comments Download
M sync/internal_api/shared_model_type_processor.cc View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M sync/internal_api/shared_model_type_processor_unittest.cc View 1 2 3 4 5 6 7 4 chunks +10 lines, -9 lines 0 comments Download
M sync/internal_api/test/fake_model_type_service.cc View 1 2 3 4 5 6 7 2 chunks +28 lines, -1 line 0 comments Download

Messages

Total messages: 47 (22 generated)
Gang Wu
PTAL
4 years, 9 months ago (2016-03-04 20:29:03 UTC) #8
skym
Creation should be triggered in 1) void DeviceInfoService::OnReadAllMetadata( 2) Somewhere in the logic Pavel is ...
4 years, 9 months ago (2016-03-04 22:12:28 UTC) #9
Gang Wu
updated https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/device_info_service_unittest.cc File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1763953002/diff/80001/components/sync_driver/device_info_service_unittest.cc#newcode211 components/sync_driver/device_info_service_unittest.cc:211: void SetProcessorAndPump() { On 2016/03/04 22:12:28, skym wrote: ...
4 years, 9 months ago (2016-03-08 20:35:25 UTC) #14
Gang Wu
updated base on discussion
4 years, 9 months ago (2016-03-11 22:42:40 UTC) #17
Gang Wu
ping
4 years, 9 months ago (2016-03-15 16:32:12 UTC) #18
maxbogue
I think you missed the very end of our discussion with this CL. I believe ...
4 years, 9 months ago (2016-03-15 17:37:33 UTC) #19
Gang Wu
updated. https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver/non_blocking_data_type_controller.cc File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/240001/components/sync_driver/non_blocking_data_type_controller.cc#newcode160 components/sync_driver/non_blocking_data_type_controller.cc:160: void NonBlockingDataTypeController::StopOnModelThread() { On 2016/03/15 17:37:33, maxbogue wrote: ...
4 years, 9 months ago (2016-03-22 00:33:39 UTC) #21
skym
https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/device_info_service.h File components/sync_driver/device_info_service.h (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/device_info_service.h#newcode74 components/sync_driver/device_info_service.h:74: syncer::ModelType type() const override; You added this to the ...
4 years, 9 months ago (2016-03-22 15:56:26 UTC) #22
Gang Wu
updated base on sky's comments. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/non_blocking_data_type_controller.cc File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/non_blocking_data_type_controller.cc#newcode81 components/sync_driver/non_blocking_data_type_controller.cc:81: syncer_v2::ModelTypeChangeProcessor* model_type_change_processor = On ...
4 years, 9 months ago (2016-03-22 17:47:51 UTC) #24
skym
https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/non_blocking_data_type_controller.cc File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/non_blocking_data_type_controller.cc#newcode81 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: ...
4 years, 9 months ago (2016-03-22 19:02:06 UTC) #25
maxbogue
Hey Gang, sorry for the wall of comments but I had a lot of concerns. ...
4 years, 9 months ago (2016-03-22 21:40:13 UTC) #26
skym
https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_service.cc File sync/api/model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/api/model_type_service.cc#newcode22 sync/api/model_type_service.cc:22: if (!change_processor_.get()) { On 2016/03/22 21:40:12, maxbogue wrote: > ...
4 years, 9 months ago (2016-03-22 22:18:18 UTC) #27
skym
https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test/fake_model_type_service.cc File sync/internal_api/test/fake_model_type_service.cc (right): https://codereview.chromium.org/1763953002/diff/320001/sync/internal_api/test/fake_model_type_service.cc#newcode14 sync/internal_api/test/fake_model_type_service.cc:14: base::Bind(&FakeModelTypeService::CreateSharedModelTypeProcessor)) {} On 2016/03/22 22:18:18, skym wrote: > On ...
4 years, 9 months ago (2016-03-22 22:32:24 UTC) #28
Gang Wu
update, did not remove OnChangeProcessorSet, will remove in next CL. https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/non_blocking_data_type_controller.cc File components/sync_driver/non_blocking_data_type_controller.cc (right): https://codereview.chromium.org/1763953002/diff/280001/components/sync_driver/non_blocking_data_type_controller.cc#newcode87 ...
4 years, 9 months ago (2016-03-24 15:56:27 UTC) #32
skym
I also hate OnChangeProcessorSet! I'm curious how you plan to get rid of it though. ...
4 years, 9 months ago (2016-03-24 16:08:52 UTC) #33
Gang Wu
Did not use overload contructor for FakeModelTypeService since it will involve a lot of work ...
4 years, 9 months ago (2016-03-24 22:18:11 UTC) #34
skym
lgtm https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver/device_info_service.cc File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver/device_info_service.cc#newcode378 components/sync_driver/device_info_service.cc:378: if (!change_processor()) { On 2016/03/24 22:18:11, Gang Wu ...
4 years, 9 months ago (2016-03-24 22:51:37 UTC) #35
maxbogue
https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver/device_info_service.cc File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver/device_info_service.cc#newcode435 components/sync_driver/device_info_service.cc:435: syncer::ModelType DeviceInfoService::type() const { On 2016/03/24 16:08:51, skym wrote: ...
4 years, 9 months ago (2016-03-24 23:28:47 UTC) #36
Gang Wu
update https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver/device_info_service.cc File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/400001/components/sync_driver/device_info_service.cc#newcode435 components/sync_driver/device_info_service.cc:435: syncer::ModelType DeviceInfoService::type() const { On 2016/03/24 23:28:47, maxbogue ...
4 years, 9 months ago (2016-03-25 02:07:00 UTC) #37
maxbogue
lgtm! Thanks for your patience with this Gang! https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver/device_info_service.cc File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver/device_info_service.cc#newcode375 components/sync_driver/device_info_service.cc:375: if ...
4 years, 9 months ago (2016-03-25 17:32:41 UTC) #38
Gang Wu
updated. https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver/device_info_service.cc File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1763953002/diff/440001/components/sync_driver/device_info_service.cc#newcode375 components/sync_driver/device_info_service.cc:375: if (metadata_records->size() > 0 || global_metadata.size() > 0) ...
4 years, 9 months ago (2016-03-25 18:08:33 UTC) #39
pavely
lgtm
4 years, 9 months ago (2016-03-25 20:26:55 UTC) #40
commit-bot: I haz the power
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
4 years, 9 months ago (2016-03-25 20:53:57 UTC) #43
commit-bot: I haz the power
Committed patchset #9 (id:460001)
4 years, 9 months ago (2016-03-25 21:00:15 UTC) #45
commit-bot: I haz the power
4 years, 9 months ago (2016-03-25 21:02:14 UTC) #47
Message was sent while issue was closed.
Patchset 9 (id:??) landed as
https://crrev.com/247ad02364179cbc1b117bfcb67d086a5456235a
Cr-Commit-Position: refs/heads/master@{#383357}

Powered by Google App Engine
This is Rietveld 408576698