Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2468423010:
[Sync] ModelTypeStore::ReadMetadataCallback now returns a MetadataBatch (Closed)
Description[Sync] ModelTypeStore::ReadMetadataCallback now returns a MetadataBatch
BUG=661847
Committed: https://crrev.com/b685c4969f6ede8f577a44de4bbfdbfe765524a4
Cr-Commit-Position: refs/heads/master@{#430525}
Patch Set 1 #Patch Set 2 : Use SyncError instead of Result. #
Total comments: 16
Patch Set 3 : Switch to VerifyMetadata. #Patch Set 4 : Use protos instead of strings for tests. #
Total comments: 10
Patch Set 5 : Address comments. #
Messages
Total messages: 33 (23 generated)
The CQ bit was checked by maxbogue@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...
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 maxbogue@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...
maxbogue@chromium.org changed reviewers: + pavely@chromium.org, skym@chromium.org
Sky, Pavel, PTAL! +cc skau@ FYI
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... File components/sync/device_info/device_info_sync_bridge.cc (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... components/sync/device_info/device_info_sync_bridge.cc:373: change_processor()->OnMetadataLoaded(error, std::move(metadata_batch)); So much less code! However, this is perpetuating the pattern where ModelTypeProcessor methods take SyncError objects. https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... File components/sync/device_info/device_info_sync_bridge.h (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... components/sync/device_info/device_info_sync_bridge.h:95: void OnReadAllMetadata(SyncError error, A little bit odd how it has to deal with Result objects on the non-metadata actions. Is the plan to ultimately convert them all? We could also just pass the store an error handler object to let it report things. I'm not sure when the service would want to take action upon error. https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl.h (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl.h:110: const ModelType type_; What do we think about removing data_prefix_/metadata_prefix_/global_metadata_key_ constants and making the functions that create them non-static now that we hang onto the type? https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl_unittest.cc (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:116: ASSERT_FALSE(sync_error.IsSet()); Doesn't seem like there a case that expects an actual error. https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:157: void ExpectMetadata(std::unique_ptr<MetadataBatch> batch, Verify? :) https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:159: std::map<std::string, std::string> expected_metadata) { expected_storage_key_to_client_tag_hash ? https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:161: batch->GetModelTypeState().encryption_key_name()); This seems wrong. Should be something like batch->GetModelTypeState().AsSerializedString(). Or |global_metadata| needs a rename. https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:163: for (auto kv : expected_metadata) { kv&
The CQ bit was checked by maxbogue@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...
https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... File components/sync/device_info/device_info_sync_bridge.cc (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... components/sync/device_info/device_info_sync_bridge.cc:373: change_processor()->OnMetadataLoaded(error, std::move(metadata_batch)); On 2016/11/05 00:45:42, skym wrote: > So much less code! > > However, this is perpetuating the pattern where ModelTypeProcessor methods take > SyncError objects. Eh, doesn't make it any harder to get rid of in the future. https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... File components/sync/device_info/device_info_sync_bridge.h (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/device_... components/sync/device_info/device_info_sync_bridge.h:95: void OnReadAllMetadata(SyncError error, On 2016/11/05 00:45:42, skym wrote: > A little bit odd how it has to deal with Result objects on the non-metadata > actions. Is the plan to ultimately convert them all? > > We could also just pass the store an error handler object to let it report > things. I'm not sure when the service would want to take action upon error. No, the plan is only to change the metadata one. The model type should have unfiltered access to the data handling but we want to make the metadata handling as easy as possible. We could pass an error handler, but then we might have to deal with lifetimes and such. This is easier for now. https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl.h (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl.h:110: const ModelType type_; On 2016/11/05 00:45:42, skym wrote: > What do we think about removing > data_prefix_/metadata_prefix_/global_metadata_key_ constants and making the > functions that create them non-static now that we hang onto the type? Unsure; I don't have a good sense of the cost of string operations in critical paths, though I guess nothing in sync is /that/ critical of a path... Either way I'd rather do it in a followup. https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl_unittest.cc (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:157: void ExpectMetadata(std::unique_ptr<MetadataBatch> batch, On 2016/11/05 00:45:43, skym wrote: > Verify? :) Whoops, forgot. Done! https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:159: std::map<std::string, std::string> expected_metadata) { On 2016/11/05 00:45:43, skym wrote: > expected_storage_key_to_client_tag_hash ? I'm keeping the weird way I'm stuffing the strings into the protos transparent to the test cases. https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:161: batch->GetModelTypeState().encryption_key_name()); On 2016/11/05 00:45:43, skym wrote: > This seems wrong. Should be something like > batch->GetModelTypeState().AsSerializedString(). Or |global_metadata| needs a > rename. Same as above. WriteGlobalMetadata just stuffs a string into that field because it's the one it can. The tests don't need to care. https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:163: for (auto kv : expected_metadata) { On 2016/11/05 00:45:42, skym wrote: > kv& const auto&, but done.
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 maxbogue@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 again to use protobufs instead of strings for the metadata tests parameters.
lgtm https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl_unittest.cc (right): https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:21: sync_pb::ModelTypeState CreateModelTypeState(const std::string& value) { Should the param be named encryption_key_name? https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:27: sync_pb::EntityMetadata CreateEntityMetadata(const std::string& value) { Should the param be named client_tag_hash?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2468423010/diff/60001/components/sync/model/m... File components/sync/model/model_type_store.h (right): https://codereview.chromium.org/2468423010/diff/60001/components/sync/model/m... components/sync/model/model_type_store.h:116: typedef base::Callback<void(SyncError sync_error, Alternatively you could introduce helper function that translates from (result, metadata_records, global_metadata) to (sync_error, metadata_batch) and call it in bridge implementation. https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl.cc (right): https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl.cc:277: } else { This function handles both verifying the right number of global metadata records and parsing metadata/verifying its correctness. I would break parsing into a helper function. https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl.cc:282: if (state.ParseFromString((*global_metadata_records)[0].value)) { Alternatively: if (!....) { callback.Run(...) return; } metadata_batch->SetModelTypeState(state); Same for body of the loop below.
The CQ bit was checked by maxbogue@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...
https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl_unittest.cc (right): https://codereview.chromium.org/2468423010/diff/20001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:116: ASSERT_FALSE(sync_error.IsSet()); On 2016/11/05 00:45:43, skym wrote: > Doesn't seem like there a case that expects an actual error. Actually did this now, whoops. https://codereview.chromium.org/2468423010/diff/60001/components/sync/model/m... File components/sync/model/model_type_store.h (right): https://codereview.chromium.org/2468423010/diff/60001/components/sync/model/m... components/sync/model/model_type_store.h:116: typedef base::Callback<void(SyncError sync_error, On 2016/11/08 00:52:08, pavely wrote: > Alternatively you could introduce helper function that translates from (result, > metadata_records, global_metadata) to (sync_error, metadata_batch) and call it > in bridge implementation. I'm satisfied with this version. I'm not entirely sure where such a function would live (I guess maybe on ModelTypeStore?), but I can't foresee a future where anyone wants to do anything with metadata stuff but immediately pass it to the processor. https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl.cc (right): https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl.cc:277: } else { On 2016/11/08 00:52:08, pavely wrote: > This function handles both verifying the right number of global metadata records > and parsing metadata/verifying its correctness. I would break parsing into a > helper function. Ah, that is cleaner. Done, thanks! https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl.cc:282: if (state.ParseFromString((*global_metadata_records)[0].value)) { On 2016/11/08 00:52:08, pavely wrote: > Alternatively: > if (!....) { > callback.Run(...) > return; > } > metadata_batch->SetModelTypeState(state); > > Same for body of the loop below. Done, here and below. https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... File components/sync/model_impl/model_type_store_impl_unittest.cc (right): https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:21: sync_pb::ModelTypeState CreateModelTypeState(const std::string& value) { On 2016/11/07 18:34:21, skym wrote: > Should the param be named encryption_key_name? Not doing this per offline discussion. https://codereview.chromium.org/2468423010/diff/60001/components/sync/model_i... components/sync/model_impl/model_type_store_impl_unittest.cc:27: sync_pb::EntityMetadata CreateEntityMetadata(const std::string& value) { On 2016/11/07 18:34:21, skym wrote: > Should the param be named client_tag_hash? Not doing this per offline discussion.
The CQ bit was unchecked by maxbogue@chromium.org
The CQ bit was checked by maxbogue@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skym@chromium.org, pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2468423010/#ps80001 (title: "Address comments.")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] ModelTypeStore::ReadMetadataCallback now returns a MetadataBatch BUG=661847 ========== to ========== [Sync] ModelTypeStore::ReadMetadataCallback now returns a MetadataBatch BUG=661847 Committed: https://crrev.com/b685c4969f6ede8f577a44de4bbfdbfe765524a4 Cr-Commit-Position: refs/heads/master@{#430525} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b685c4969f6ede8f577a44de4bbfdbfe765524a4 Cr-Commit-Position: refs/heads/master@{#430525} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
