Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2168273002:
[Sync] Fix behavior when there are two type roots for a type (Closed)
|
Created:
4 years, 5 months ago by Nicolas Zea Modified:
4 years, 5 months ago CC:
chromium-reviews, sync-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] Fix behavior when there are two type roots for a type
Due to a bug in how server generated root nodes for autofill wallet metadata
were created, clients can have both an implicitly created root node and an
explicitly (server-side) created root node. This change makes it so we
gracefully handle this by allowing both to live side by side in the
parent_child_index, and ensures new entities don't become invisible.
BUG=630035
Committed: https://crrev.com/ca751c9fd46d972b9e18aff7871df323b1cccaff
Cr-Commit-Position: refs/heads/master@{#407544}
Patch Set 1 #Patch Set 2 : Fix memory lifetimes #Patch Set 3 : Self review #
Total comments: 3
Patch Set 4 : Update comment #
Total comments: 4
Patch Set 5 : Fix nit #
Messages
Total messages: 26 (16 generated)
Description was changed from ========== [Sync] Fix behavior when there are two type roots for a type Due to a bug in how server generated root nodes for autofill wallet metadata were created, clients can have both an implicitly created root node and an explicitly (server-side) created root node. This change makes it so we always (and consistently) favor implicitly created root nodes over explicit ones. BUG=630035 ========== to ========== [Sync] Fix behavior when there are two type roots for a type Due to a bug in how server generated root nodes for autofill wallet metadata were created, clients can have both an implicitly created root node and an explicitly (server-side) created root node. This change makes it so we gracefully handle this by allowing both to live side by side in the parent_child_index, and ensures new entities don't become invisible. BUG=630035 ==========
The CQ bit was checked by zea@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 checked by zea@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...
zea@chromium.org changed reviewers: + pnoland@chromium.org, stanisc@chromium.org
+Patrick for sync review +Stan, since you made these changes originally could you also double check my logic?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with one comment https://codereview.chromium.org/2168273002/diff/40001/sync/syncable/parent_ch... File sync/syncable/parent_child_index.cc (right): https://codereview.chromium.org/2168273002/diff/40001/sync/syncable/parent_ch... sync/syncable/parent_child_index.cc:93: // in use, so we keep it in the parent_children_map_. This comment is kind of confusing without the context of the previously existing code, since it's explaining why're -not- doing a non-obvious operation. I think it's enough to just acknowledge that multiple roots could come to exist side by side, and explain why that's OK.
https://codereview.chromium.org/2168273002/diff/40001/sync/syncable/parent_ch... File sync/syncable/parent_child_index.cc (right): https://codereview.chromium.org/2168273002/diff/40001/sync/syncable/parent_ch... sync/syncable/parent_child_index.cc:93: // in use, so we keep it in the parent_children_map_. Just to make sure if I understand this correctly. We keep the old entry in the map but both the old one and the new one end up pointing to the same shared OrderedChildSet, right?
The CQ bit was checked by zea@chromium.org to run a CQ dry run
https://codereview.chromium.org/2168273002/diff/40001/sync/syncable/parent_ch... File sync/syncable/parent_child_index.cc (right): https://codereview.chromium.org/2168273002/diff/40001/sync/syncable/parent_ch... sync/syncable/parent_child_index.cc:93: // in use, so we keep it in the parent_children_map_. On 2016/07/22 22:17:29, stanisc wrote: > Just to make sure if I understand this correctly. We keep the old entry in the > map but both the old one and the new one end up pointing to the same shared > OrderedChildSet, right? That's correct. Also updated the comment based on Patrick's comment.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
A couple of nitpicks: https://codereview.chromium.org/2168273002/diff/60001/sync/syncable/parent_ch... File sync/syncable/parent_child_index_unittest.cc (right): https://codereview.chromium.org/2168273002/diff/60001/sync/syncable/parent_ch... sync/syncable/parent_child_index_unittest.cc:473: // Test that the if for some reason we wind up with multiple type roots, we "Test that the if" https://codereview.chromium.org/2168273002/diff/60001/sync/syncable/parent_ch... sync/syncable/parent_child_index_unittest.cc:498: // Test that the if for some reason we wind up with multiple type roots, we "Test that the if"
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Done, thanks! https://codereview.chromium.org/2168273002/diff/60001/sync/syncable/parent_ch... File sync/syncable/parent_child_index_unittest.cc (right): https://codereview.chromium.org/2168273002/diff/60001/sync/syncable/parent_ch... sync/syncable/parent_child_index_unittest.cc:473: // Test that the if for some reason we wind up with multiple type roots, we On 2016/07/22 23:50:25, stanisc wrote: > "Test that the if" Done. https://codereview.chromium.org/2168273002/diff/60001/sync/syncable/parent_ch... sync/syncable/parent_child_index_unittest.cc:498: // Test that the if for some reason we wind up with multiple type roots, we On 2016/07/22 23:50:25, stanisc wrote: > "Test that the if" Done.
The CQ bit was checked by zea@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pnoland@chromium.org, stanisc@chromium.org Link to the patchset: https://codereview.chromium.org/2168273002/#ps80001 (title: "Fix nit")
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] Fix behavior when there are two type roots for a type Due to a bug in how server generated root nodes for autofill wallet metadata were created, clients can have both an implicitly created root node and an explicitly (server-side) created root node. This change makes it so we gracefully handle this by allowing both to live side by side in the parent_child_index, and ensures new entities don't become invisible. BUG=630035 ========== to ========== [Sync] Fix behavior when there are two type roots for a type Due to a bug in how server generated root nodes for autofill wallet metadata were created, clients can have both an implicitly created root node and an explicitly (server-side) created root node. This change makes it so we gracefully handle this by allowing both to live side by side in the parent_child_index, and ensures new entities don't become invisible. BUG=630035 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Sync] Fix behavior when there are two type roots for a type Due to a bug in how server generated root nodes for autofill wallet metadata were created, clients can have both an implicitly created root node and an explicitly (server-side) created root node. This change makes it so we gracefully handle this by allowing both to live side by side in the parent_child_index, and ensures new entities don't become invisible. BUG=630035 ========== to ========== [Sync] Fix behavior when there are two type roots for a type Due to a bug in how server generated root nodes for autofill wallet metadata were created, clients can have both an implicitly created root node and an explicitly (server-side) created root node. This change makes it so we gracefully handle this by allowing both to live side by side in the parent_child_index, and ensures new entities don't become invisible. BUG=630035 Committed: https://crrev.com/ca751c9fd46d972b9e18aff7871df323b1cccaff Cr-Commit-Position: refs/heads/master@{#407544} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ca751c9fd46d972b9e18aff7871df323b1cccaff Cr-Commit-Position: refs/heads/master@{#407544} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
