Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2935583002:
[Sync] Implement support for untracking new entities (Closed)
Description[Sync] Implement support for untracking new entities
This is a follow up CL for https://codereview.chromium.org/2915763005.
The CL above let processor can obtain storage key during bridge merge/apply,
but some entities maybe expired and bridge do not want to persist them, so this CL
add a function called UntrackEntity in processor to let processor to remove
tracker for those entities.
BUG=719570
Review-Url: https://codereview.chromium.org/2935583002
Cr-Commit-Position: refs/heads/master@{#480096}
Committed: https://chromium.googlesource.com/chromium/src/+/20065cfbc168c8a6d919196499c7ac484351c5d8
Patch Set 1 #
Total comments: 12
Patch Set 2 : pavely review #
Total comments: 8
Patch Set 3 : add a new GenerateUpdateData #
Total comments: 2
Patch Set 4 : add comments #Messages
Total messages: 44 (35 generated)
The CQ bit was checked by gangwu@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by gangwu@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.
Description was changed from ========== add unittests BUG= ========== to ========== [Sync] Implement support for untracking new entities This is a follow up CL for https://codereview.chromium.org/2915763005. The CL above let processor can obtain storage key during bridge merge/apply, but some entities maybe expired and bridge do not want to persist them, so this CL add a function called UntrackEntity in processor to let processor to remove tracker for those entities. BUG=719570 ==========
gangwu@chromium.org changed reviewers: + pavely@chromium.org
Patchset #1 (id:1) has been deleted
PTAL
https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/f... File components/sync/model/fake_model_type_sync_bridge.cc (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.cc:58: const char FakeModelTypeSyncBridge::kUntrackEntityKeyPrefix[] = "untrack"; You are setting up this class to ignore entities based on prefix, this is hidden dependency. With such approach you need to format preference names in the test in a special way and explain in a comment why you are doing it this way. It would be more flexible if FakeModelTypeSyncBridge had a method SetKeysToIgnore(). In this case individual tests would decide if they want bridge to ignore any keys and would express it with explicit call from the test. https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... File components/sync/model/model_type_change_processor.h (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... components/sync/model/model_type_change_processor.h:61: // Remove entity metadata and do not tracking the entity. This function only tracking => track https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... components/sync/model/model_type_change_processor.h:64: // MergeSyncData/ApplySyncChanges to inform the processor about this entity about => that https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... components/sync/model/model_type_change_processor.h:65: // should not been tracked. For the datatypes which can generate storage key For the datatypes .... => Datatypes that support GetStorageKey should call change_processor()->Delete() instead. https://codereview.chromium.org/2935583002/diff/20001/components/sync/model_i... File components/sync/model_impl/shared_model_type_processor_unittest.cc (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor_unittest.cc:48: std::string(FakeModelTypeSyncBridge::kUntrackEntityKeyPrefix) + "1"); See my comment in fake_model_type_sync_bridge.cc https://codereview.chromium.org/2935583002/diff/20001/components/sync/test/en... File components/sync/test/engine/mock_model_type_worker.h (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/test/en... components/sync/test/engine/mock_model_type_worker.h:60: void UpdateFromServer(const std::vector<std::string>& tag_hash, These functions used to take single hash and specifics, it was obvious they belonged to the same entity. Changing them to take two vectors makes this relationship obscure. I think better solution would be to introduce function that takes vector of UpdateResponseData similar to UpdateWithEncryptionKey only without encryption key. To simplify calling this function from test you can add GenerateUpdateData variant that only takes tag_hash and specifics and uses default version offset and encryption key name.
The CQ bit was checked by gangwu@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...
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by gangwu@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: Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by gangwu@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...
Patchset #2 (id:60001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
updated. https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/f... File components/sync/model/fake_model_type_sync_bridge.cc (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.cc:58: const char FakeModelTypeSyncBridge::kUntrackEntityKeyPrefix[] = "untrack"; On 2017/06/13 23:09:39, pavely wrote: > You are setting up this class to ignore entities based on prefix, this is hidden > dependency. With such approach you need to format preference names in the test > in a special way and explain in a comment why you are doing it this way. > > It would be more flexible if FakeModelTypeSyncBridge had a method > SetKeysToIgnore(). In this case individual tests would decide if they want > bridge to ignore any keys and would express it with explicit call from the test. Done. https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... File components/sync/model/model_type_change_processor.h (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... components/sync/model/model_type_change_processor.h:61: // Remove entity metadata and do not tracking the entity. This function only On 2017/06/13 23:09:39, pavely wrote: > tracking => track Done. https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... components/sync/model/model_type_change_processor.h:64: // MergeSyncData/ApplySyncChanges to inform the processor about this entity On 2017/06/13 23:09:39, pavely wrote: > about => that Done. https://codereview.chromium.org/2935583002/diff/20001/components/sync/model/m... components/sync/model/model_type_change_processor.h:65: // should not been tracked. For the datatypes which can generate storage key On 2017/06/13 23:09:39, pavely wrote: > For the datatypes .... => > Datatypes that support GetStorageKey should call change_processor()->Delete() > instead. Done. https://codereview.chromium.org/2935583002/diff/20001/components/sync/model_i... File components/sync/model_impl/shared_model_type_processor_unittest.cc (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor_unittest.cc:48: std::string(FakeModelTypeSyncBridge::kUntrackEntityKeyPrefix) + "1"); On 2017/06/13 23:09:39, pavely wrote: > See my comment in fake_model_type_sync_bridge.cc Done. https://codereview.chromium.org/2935583002/diff/20001/components/sync/test/en... File components/sync/test/engine/mock_model_type_worker.h (right): https://codereview.chromium.org/2935583002/diff/20001/components/sync/test/en... components/sync/test/engine/mock_model_type_worker.h:60: void UpdateFromServer(const std::vector<std::string>& tag_hash, On 2017/06/13 23:09:39, pavely wrote: > These functions used to take single hash and specifics, it was obvious they > belonged to the same entity. Changing them to take two vectors makes this > relationship obscure. I think better solution would be to introduce function > that takes vector of UpdateResponseData similar to UpdateWithEncryptionKey only > without encryption key. To simplify calling this function from test you can add > GenerateUpdateData variant that only takes tag_hash and specifics and uses > default version offset and encryption key name. Done.
https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... File components/sync/model/fake_model_type_sync_bridge.cc (right): https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.cc:216: if (key_to_ignore_.find(storage_key) != key_to_ignore_.end()) { There are some helper functions in stl_util.h. This pattern is covered by base::ContainsKey(). https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... File components/sync/model/fake_model_type_sync_bridge.h (right): https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.h:154: // THe storage keys which bridge will ignore. THe => The https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.h:155: std::unordered_set<std::string> key_to_ignore_; key_to_ignore_ => keys_to_ignore_ https://codereview.chromium.org/2935583002/diff/80001/components/sync/model_i... File components/sync/model_impl/shared_model_type_processor_unittest.cc (right): https://codereview.chromium.org/2935583002/diff/80001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor_unittest.cc:1457: updates.push_back(worker()->GenerateUpdateData( What I meant was to introduce another GenerateUpdateData that would take hash and specifics (no other parameters) and use default version offset and encryption key name. This code would look something like: updates.push_back(worker()->GenerateUpdateData(kHash1, GenerateSpecifics(kKey1, kValue1)); The goal is to let reader read only relevant parts (update contains entry for kHash1 with specifics for kKey1, kValue1) and remove all the repetitive parts that have nothing to do with test (, 1, worker()->model_type_state().encryption_key_name())
The CQ bit was checked by gangwu@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, PTAL https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... File components/sync/model/fake_model_type_sync_bridge.cc (right): https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.cc:216: if (key_to_ignore_.find(storage_key) != key_to_ignore_.end()) { On 2017/06/15 19:35:33, pavely wrote: > There are some helper functions in stl_util.h. This pattern is covered by > base::ContainsKey(). Done. https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... File components/sync/model/fake_model_type_sync_bridge.h (right): https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.h:154: // THe storage keys which bridge will ignore. On 2017/06/15 19:35:33, pavely wrote: > THe => The Done. https://codereview.chromium.org/2935583002/diff/80001/components/sync/model/f... components/sync/model/fake_model_type_sync_bridge.h:155: std::unordered_set<std::string> key_to_ignore_; On 2017/06/15 19:35:33, pavely wrote: > key_to_ignore_ => keys_to_ignore_ Done. https://codereview.chromium.org/2935583002/diff/80001/components/sync/model_i... File components/sync/model_impl/shared_model_type_processor_unittest.cc (right): https://codereview.chromium.org/2935583002/diff/80001/components/sync/model_i... components/sync/model_impl/shared_model_type_processor_unittest.cc:1457: updates.push_back(worker()->GenerateUpdateData( On 2017/06/15 19:35:33, pavely wrote: > What I meant was to introduce another GenerateUpdateData that would take hash > and specifics (no other parameters) and use default version offset and > encryption key name. > This code would look something like: > updates.push_back(worker()->GenerateUpdateData(kHash1, GenerateSpecifics(kKey1, > kValue1)); The goal is to let reader read only relevant parts (update contains > entry for kHash1 with specifics for kKey1, kValue1) and remove all the > repetitive parts that have nothing to do with test (, 1, > worker()->model_type_state().encryption_key_name()) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2935583002/diff/100001/components/sync/test/e... File components/sync/test/engine/mock_model_type_worker.h (right): https://codereview.chromium.org/2935583002/diff/100001/components/sync/test/e... components/sync/test/engine/mock_model_type_worker.h:84: UpdateResponseData GenerateUpdateData( Add comment explaining which version_offset and encryption key name is used.
The CQ bit was checked by gangwu@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by gangwu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pavely@chromium.org Link to the patchset: https://codereview.chromium.org/2935583002/#ps120001 (title: "add comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497630904731820,
"parent_rev": "4704a57b1bde5279f8643c2db0b2778430e43e70", "commit_rev":
"20065cfbc168c8a6d919196499c7ac484351c5d8"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] Implement support for untracking new entities This is a follow up CL for https://codereview.chromium.org/2915763005. The CL above let processor can obtain storage key during bridge merge/apply, but some entities maybe expired and bridge do not want to persist them, so this CL add a function called UntrackEntity in processor to let processor to remove tracker for those entities. BUG=719570 ========== to ========== [Sync] Implement support for untracking new entities This is a follow up CL for https://codereview.chromium.org/2915763005. The CL above let processor can obtain storage key during bridge merge/apply, but some entities maybe expired and bridge do not want to persist them, so this CL add a function called UntrackEntity in processor to let processor to remove tracker for those entities. BUG=719570 Review-Url: https://codereview.chromium.org/2935583002 Cr-Commit-Position: refs/heads/master@{#480096} Committed: https://chromium.googlesource.com/chromium/src/+/20065cfbc168c8a6d919196499c7... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/20065cfbc168c8a6d919196499c7...
Message was sent while issue was closed.
https://codereview.chromium.org/2935583002/diff/100001/components/sync/test/e... File components/sync/test/engine/mock_model_type_worker.h (right): https://codereview.chromium.org/2935583002/diff/100001/components/sync/test/e... components/sync/test/engine/mock_model_type_worker.h:84: UpdateResponseData GenerateUpdateData( On 2017/06/15 22:38:32, pavely wrote: > Add comment explaining which version_offset and encryption key name is used. Done. |
