Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2360283003:
[Sync] When a data type is not initialized, also should not be registered with backend. (Closed)
Description[Sync] When a data type is not initialized, also should not be registered with backend.
The switch from queue to deque is to allow iterating over the data structure.
BUG=647875
Committed: https://crrev.com/a842013d816fd4a6c5855bffd65a0a7e27cfdd02
Cr-Commit-Position: refs/heads/master@{#421095}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Switched to GetEnabledTypes() #
Total comments: 1
Patch Set 3 : Back to member variable, since as Pavel pointed out, the status map can change. #
Messages
Total messages: 26 (17 generated)
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Description was changed from ========== [Sync] When a data type is not initialized, also should not be registered with backend. BUG=647875 ========== to ========== [Sync] When a data type is not initialized, also should not be registered with backend. The switch from queue to deque is to allow iterating over the data structure. BUG=647875 ==========
skym@chromium.org changed reviewers: + pavely@chromium.org
PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm Consider suggestions I left. Either way is fine with me. https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... File components/sync/driver/data_type_manager_impl.cc (right): https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... components/sync/driver/data_type_manager_impl.cc:170: for (const syncer::ModelTypeSet& set : download_types_deque_) { It is not immediately obvious that you are enumerating enabled types. Primary purpose of download types queue is to track order in which data types will be downloaded/associated. I propose to either add enabled_types_ field to DataTypeManagerImpl or to add comment here explaining intention. https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... File components/sync/driver/data_type_manager_impl.h (right): https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... components/sync/driver/data_type_manager_impl.h:188: TypeSetPriorityList download_types_deque_; I think word queue in field name is not to convey C++ construct backing it but the semantics (it is queue of sets of types to be downloaded). I think it will be more readable to keep original name.
The CQ bit was checked by skym@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated for comments, made a GetEnabledTypes() accessor instead. https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... File components/sync/driver/data_type_manager_impl.cc (right): https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... components/sync/driver/data_type_manager_impl.cc:170: for (const syncer::ModelTypeSet& set : download_types_deque_) { On 2016/09/26 17:50:48, pavely wrote: > It is not immediately obvious that you are enumerating enabled types. Primary > purpose of download types queue is to track order in which data types will be > downloaded/associated. > I propose to either add enabled_types_ field to DataTypeManagerImpl or to add > comment here explaining intention. Tried to add |enabled_types_|, but found there was an |enabled_types| in two places. They both created the set the same way syncer::Difference(last_requested_types_, data_type_status_table_.GetFailedTypes() So I've instead created an accessor to do that. I'm pretty sure it's safe, but I could be wrong. https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... File components/sync/driver/data_type_manager_impl.h (right): https://codereview.chromium.org/2360283003/diff/1/components/sync/driver/data... components/sync/driver/data_type_manager_impl.h:188: TypeSetPriorityList download_types_deque_; On 2016/09/26 17:50:48, pavely wrote: > I think word queue in field name is not to convey C++ construct backing it but > the semantics (it is queue of sets of types to be downloaded). I think it will > be more readable to keep original name. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2360283003/diff/20001/components/sync/driver/... File components/sync/driver/data_type_manager_impl.h (right): https://codereview.chromium.org/2360283003/diff/20001/components/sync/driver/... components/sync/driver/data_type_manager_impl.h:157: syncer::ModelTypeSet enabled_types_; Hmm, this should have been deleted.
The CQ bit was checked by skym@chromium.org to run a CQ dry run
PTAL.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by skym@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [Sync] When a data type is not initialized, also should not be registered with backend. The switch from queue to deque is to allow iterating over the data structure. BUG=647875 ========== to ========== [Sync] When a data type is not initialized, also should not be registered with backend. The switch from queue to deque is to allow iterating over the data structure. BUG=647875 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] When a data type is not initialized, also should not be registered with backend. The switch from queue to deque is to allow iterating over the data structure. BUG=647875 ========== to ========== [Sync] When a data type is not initialized, also should not be registered with backend. The switch from queue to deque is to allow iterating over the data structure. BUG=647875 Committed: https://crrev.com/a842013d816fd4a6c5855bffd65a0a7e27cfdd02 Cr-Commit-Position: refs/heads/master@{#421095} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/a842013d816fd4a6c5855bffd65a0a7e27cfdd02 Cr-Commit-Position: refs/heads/master@{#421095} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
