Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1712863002:
[Sync] DeviceInfo implementation of MergeSyncData. (Closed)
|
Created:
4 years, 10 months ago by skym 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@diapply Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] DeviceInfo implementation of MergeSyncData.
BUG=543404, 543405, 543406
Committed: https://crrev.com/0a4cdc91f9c415a44bb2dc8e884b1bc311805bde
Cr-Commit-Position: refs/heads/master@{#379578}
Patch Set 1 : #
Total comments: 12
Patch Set 2 : #
Messages
Total messages: 23 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== [Sync] DeviceInfo implementation of MergeSyncData. BUG=543404, 543405, 543406 ========== to ========== [Sync] DeviceInfo implementation of MergeSyncData. BUG=543404, 543405, 543406 ==========
skym@chromium.org changed reviewers: + gangwu@chromium.org, maxbogue@chromium.org, pavely@chromium.org
ptal
https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:120: base::Bind(&DeviceInfoService::OnCommit, weak_factory_.GetWeakPtr())); Should be calling NotifyObservers(); at the end of this function if any change occurred.
lgtm w/ comments, even though you'll have to change a bit to resolve conflicts with http://crrev.com/1717903002. https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:87: for (auto& kv : all_data_) { "const auto&"? https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:94: for (EntityDataPtr& entity_data_ptr : entity_data_list) { "const EntityDataPtr&"? https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:111: change_processor()->Put(tag, CopyIntoNewEntityData(*all_data_[tag].get()), Nit: pretty sure the .get() is superfluous here and you can just deref a scoped_ptr. https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service_unittest.cc:617: Add a TODO here to actually commit |metadata_changes|? I was confused why this whole block existed until I saw the bit at the bottom.
lgtm
lgtm
https://codereview.chromium.org/1712863002/diff/80001/sync/api/model_type_ser... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1712863002/diff/80001/sync/api/model_type_ser... sync/api/model_type_service.h:42: // to be called when sync is first turned on, not on every restart. An attempt This comment also needs to include: * What the keys are in the map, how they were created. * The order persistent writes needs to occur in should they not be done atomically.
Patchset #2 (id:100001) has been deleted
Patchset #2 (id:120001) has been deleted
Rebase and updates, Pavel - can you take another glace at this? https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... File components/sync_driver/device_info_service.cc (right): https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:87: for (auto& kv : all_data_) { On 2016/02/23 01:04:02, maxbogue wrote: > "const auto&"? Done. https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:94: for (EntityDataPtr& entity_data_ptr : entity_data_list) { On 2016/02/23 01:04:02, maxbogue wrote: > "const EntityDataPtr&"? Done. https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:111: change_processor()->Put(tag, CopyIntoNewEntityData(*all_data_[tag].get()), On 2016/02/23 01:04:02, maxbogue wrote: > Nit: pretty sure the .get() is superfluous here and you can just deref a > scoped_ptr. TIL. Done. https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service.cc:120: base::Bind(&DeviceInfoService::OnCommit, weak_factory_.GetWeakPtr())); On 2016/02/22 17:43:10, skym wrote: > Should be calling NotifyObservers(); at the end of this function if any change > occurred. Done. https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... File components/sync_driver/device_info_service_unittest.cc (right): https://codereview.chromium.org/1712863002/diff/80001/components/sync_driver/... components/sync_driver/device_info_service_unittest.cc:617: On 2016/02/23 01:04:02, maxbogue wrote: > Add a TODO here to actually commit |metadata_changes|? I was confused why this > whole block existed until I saw the bit at the bottom. Whoopps, it should be getting passed into MergeSyncData, instead of a second one getting created. Fixed. Nothing up here should need to change after TransferChanges gets implemented (if that's how we decide to do it). https://codereview.chromium.org/1712863002/diff/80001/sync/api/model_type_ser... File sync/api/model_type_service.h (right): https://codereview.chromium.org/1712863002/diff/80001/sync/api/model_type_ser... sync/api/model_type_service.h:42: // to be called when sync is first turned on, not on every restart. An attempt On 2016/02/23 20:53:28, skym wrote: > This comment also needs to include: > * What the keys are in the map, how they were created. > * The order persistent writes needs to occur in should they not be done > atomically. Done.
lgtm
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from maxbogue@chromium.org, gangwu@chromium.org Link to the patchset: https://codereview.chromium.org/1712863002/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1712863002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1712863002/140001
Message was sent while issue was closed.
Description was changed from ========== [Sync] DeviceInfo implementation of MergeSyncData. BUG=543404, 543405, 543406 ========== to ========== [Sync] DeviceInfo implementation of MergeSyncData. BUG=543404, 543405, 543406 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] DeviceInfo implementation of MergeSyncData. BUG=543404, 543405, 543406 ========== to ========== [Sync] DeviceInfo implementation of MergeSyncData. BUG=543404, 543405, 543406 Committed: https://crrev.com/0a4cdc91f9c415a44bb2dc8e884b1bc311805bde Cr-Commit-Position: refs/heads/master@{#379578} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/0a4cdc91f9c415a44bb2dc8e884b1bc311805bde Cr-Commit-Position: refs/heads/master@{#379578} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
