Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2624883002:
[Sync] ApplySyncChanges autofill implementation. (Closed)
|
Created:
3 years, 11 months ago by skym Modified:
3 years, 11 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Sync] ApplySyncChanges autofill implementation.
Updates autofill USS implementation to accept incoming sync changes. The
previous implementation can be found at
AutocompleteSyncableService::ProcessSyncChanges, which was used to base
the behavior off of for this CL. I've tried to setup the code in such a
way that allows apply and merge to share as much code as possible. I
tried to keep the overhead/complexity to a minimum, not quite sure how
successful I have been. Some of the odd/gnarly pieces of logic that
were (or not) copied over that I think may be worth calling out:
1. AutofillTable::GetAllAutofillEntries is only called if needed. While
the processor shouldn't call us with no changes for ApplySyncChanges,
it's up to us to avoid this when we're only receiving deletes. This is
the previous behavior.
2. AutofillSpecifics should be ignored when .has_value() returns false.
These are dropped on the floor with no attempt to delete. As far back
as I could find, like over 5 years ago, this has been the case. No
special handling exists for .has_name(), which can return false and
we'll treat it as normal. This is the previous behavior.
3. Don't trust the incoming time stamps from Sync. This is a change
from how AutocompleteSyncableService was implemented. In an attempt to
be conservative in what we send, and liberal in what we accept, I've
tried to add the ability to handle incorrectly ordered timestamps, and
correct them locally. The cost of this should be negligible since
typically there should be 1 or 2 timestmaps on any given remote
specifics.
4. Converting from an AutofillSpecifics with no timestamps is kind of
invalid, we end up setting our creation timestamp to time 0. We
explicitly avoid this scenario when updating existing data, but when
the sync change is the first time we've seen this data, we don't have
quite as reasonable of an option, so we set creation and last use time
to time 0. This is going to result in an immediate eviction by
RemoveExpiredFormElements, but this the previous behavior, so I kept
it.
BUG=672619
Review-Url: https://codereview.chromium.org/2624883002
Cr-Commit-Position: refs/heads/master@{#443601}
Committed: https://chromium.googlesource.com/chromium/src/+/0299ea23ebf462d0bec0d9571711b8d6c437cb8c
Patch Set 1 #Patch Set 2 : Increased state tracking and logic in sub-class. #Patch Set 3 : Cleaned up a few things, added missing checks to unittests. #Patch Set 4 : Rebasing and removed a few useless autos. #
Total comments: 30
Patch Set 5 : Updates for Max. #
Total comments: 8
Patch Set 6 : More updates for Max's comments. #
Messages
Total messages: 31 (23 generated)
Description was changed from ========== ApplySyncChanges autofill implementation. BUG=672619 ========== to ========== [Sync] ApplySyncChanges autofill implementation. BUG=672619 ==========
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [Sync] ApplySyncChanges autofill implementation. BUG=672619 ========== to ========== [Sync] ApplySyncChanges autofill implementation. This the USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Sort the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. I an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from a AutofillSpecifics with 0 timestamps is kind of invalid, we end up setting our creating timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ==========
skym@chromium.org changed reviewers: + maxbogue@chromium.org
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Mostly looks good! Just a few things to address. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:46: #define RETURN_IF(x) \ Reading through where this is used, I think I'd prefer it to be RETURN_IF_ERROR. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:83: AutofillEntry MergeEntryDates(const AutofillEntry& first, Honestly variable names like a and b would probably be just as clear here. Or entry1 and entry2 if you want to make them more descriptive. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:109: std::vector<int64_t> sorted(timestamps.begin(), timestamps.end()); Consider using std::minmax_element (http://en.cppreference.com/w/cpp/algorithm/minmax_element) to get the min and max elements without needing to construct a separate list or sort it. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:198: // This should never be true because only ApplySyncChanges should be I don't believe this is true; deletes can come through merge (https://cs.chromium.org/chromium/src/components/sync/model/entity_data.h?l=73). https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:199: // calling IncorporateRemoteDelete. While only MergeSyncData should be s/. While/, while https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:222: std::set<AutofillEntry>::iterator iter = auto https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:249: // |unique_to_local_| should typically be done through ReadEntry(). s/should typically/must https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:261: // Contains current data for entries that are know to exist on both sync and Maybe "Contains merged data for entries that existed on both sync and local sides and need to be saved back to sync."? https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:172: timestamps); std::move(timestamps)? Or make it a const& param in both functions https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:196: void VerifyApplyChanges(const std::vector<EntityChange> changes) { const ref instead of just const? https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:197: const base::Optional<ModelError> error = bridge()->ApplySyncChanges( const auto error probs https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:417: const base::Optional<ModelError> error = bridge()->ApplySyncChanges( const auto error? https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_entry.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_entry.cc:58: bool AutofillEntry::operator!=(const AutofillEntry& entry) const { How does this not come with operator==...? Oh C++... https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_metadata_change_list.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_metadata_change_list.cc:71: error_ = Optional<ModelError>(); can do error_.reset() too. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_metadata_change_list.h (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_metadata_change_list.h:40: base::Optional<syncer::ModelError> TakeError(); Holy smokes I missed some!
Description was changed from ========== [Sync] ApplySyncChanges autofill implementation. This the USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Sort the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. I an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from a AutofillSpecifics with 0 timestamps is kind of invalid, we end up setting our creating timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ========== to ========== [Sync] ApplySyncChanges autofill implementation. This the USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Don't trust the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. I an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from a AutofillSpecifics with 0 timestamps is kind of invalid, we end up setting our creating timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ==========
Description was changed from ========== [Sync] ApplySyncChanges autofill implementation. This the USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Don't trust the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. I an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from a AutofillSpecifics with 0 timestamps is kind of invalid, we end up setting our creating timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ========== to ========== [Sync] ApplySyncChanges autofill implementation. This the USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Don't trust the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. In an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from an AutofillSpecifics with no timestamps is kind of invalid, we end up setting our creation timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ==========
The CQ bit was checked by skym@chromium.org to run a CQ dry run
skym@chromium.org changed reviewers: + mathp@chromium.org
Adding mathp@ as autofill owner. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:46: #define RETURN_IF(x) \ On 2017/01/12 00:15:16, maxbogue wrote: > Reading through where this is used, I think I'd prefer it to be RETURN_IF_ERROR. Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:83: AutofillEntry MergeEntryDates(const AutofillEntry& first, On 2017/01/12 00:15:16, maxbogue wrote: > Honestly variable names like a and b would probably be just as clear here. Or > entry1 and entry2 if you want to make them more descriptive. Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:109: std::vector<int64_t> sorted(timestamps.begin(), timestamps.end()); On 2017/01/12 00:15:16, maxbogue wrote: > Consider using std::minmax_element > (http://en.cppreference.com/w/cpp/algorithm/minmax_element) to get the min and > max elements without needing to construct a separate list or sort it. Love it! Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:198: // This should never be true because only ApplySyncChanges should be On 2017/01/12 00:15:16, maxbogue wrote: > I don't believe this is true; deletes can come through merge > (https://cs.chromium.org/chromium/src/components/sync/model/entity_data.h?l=73). Sigh... Yeah, I messed this up (great catch btw Max). Didn't realize we had to deal with deletes during merge. I think all the existing USS merge implementations to date have messed this up. https://cs.chromium.org/chromium/src/components/sync/device_info/device_info_... https://cs.chromium.org/chromium/src/components/reading_list/ios/reading_list... https://codereview.chromium.org/2432803003/diff/120001/chrome/browser/chromeo... How did the directory merge work anyways? We should investigate. It makes sense from the perspective that a client might be repeatedly enabling-disabling sync to accept tombstones. But I'm not sure it makes sense from a first connection standpoint to delete things that happen to match. Created crbug.com/680550, added a TODO to the merge function. I'm going to leave this comment as is for now. Best case, we decide merge should never expect tombstones :) https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:199: // calling IncorporateRemoteDelete. While only MergeSyncData should be On 2017/01/12 00:15:16, maxbogue wrote: > s/. While/, while Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:222: std::set<AutofillEntry>::iterator iter = On 2017/01/12 00:15:16, maxbogue wrote: > auto Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:249: // |unique_to_local_| should typically be done through ReadEntry(). On 2017/01/12 00:15:16, maxbogue wrote: > s/should typically/must I'm going to keep "should typically". The thing is, if what you want to accomplish isn't reading a single entry (see FlushToSync()), then ReadEntry doesn't work. It is awkward, but at least it's all contained within a fairly small class. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:261: // Contains current data for entries that are know to exist on both sync and On 2017/01/12 00:15:16, maxbogue wrote: > Maybe "Contains merged data for entries that existed on both sync and local > sides and need to be saved back to sync."? Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:172: timestamps); On 2017/01/12 00:15:17, maxbogue wrote: > std::move(timestamps)? Or make it a const& param in both functions Made it const&. Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:196: void VerifyApplyChanges(const std::vector<EntityChange> changes) { On 2017/01/12 00:15:16, maxbogue wrote: > const ref instead of just const? Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:197: const base::Optional<ModelError> error = bridge()->ApplySyncChanges( On 2017/01/12 00:15:17, maxbogue wrote: > const auto error probs Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge_unittest.cc:417: const base::Optional<ModelError> error = bridge()->ApplySyncChanges( On 2017/01/12 00:15:17, maxbogue wrote: > const auto error? Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_entry.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_entry.cc:58: bool AutofillEntry::operator!=(const AutofillEntry& entry) const { On 2017/01/12 00:15:17, maxbogue wrote: > How does this not come with operator==...? Oh C++... Acknowledged. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_metadata_change_list.cc (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_metadata_change_list.cc:71: error_ = Optional<ModelError>(); On 2017/01/12 00:15:17, maxbogue wrote: > can do error_.reset() too. Oooh, great idea. Done. https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... File components/autofill/core/browser/webdata/autofill_metadata_change_list.h (right): https://codereview.chromium.org/2624883002/diff/60001/components/autofill/cor... components/autofill/core/browser/webdata/autofill_metadata_change_list.h:40: base::Optional<syncer::ModelError> TakeError(); On 2017/01/12 00:15:17, maxbogue wrote: > Holy smokes I missed some! :)
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Sync] ApplySyncChanges autofill implementation. This the USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Don't trust the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. In an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from an AutofillSpecifics with no timestamps is kind of invalid, we end up setting our creation timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ========== to ========== [Sync] ApplySyncChanges autofill implementation. Updates autofill USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Don't trust the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. In an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from an AutofillSpecifics with no timestamps is kind of invalid, we end up setting our creation timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:117: // to lazily load local data, and then react to sync data by maintain internal maintaining https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:247: // |unique_to_local_| should typically be done through ReadEntry(). My previous comment here was wrong. What I meant to say was that InitializeIfNeeded() must be called before unique_to_local_ is valid, right? You should clarify that here or in the unique_to_local_ comment. https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:250: // Important to note that because AutofillEntry's operator > simply compares the most nit of nits: s/>/< https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:315: // TODO(skym, crbug.com/680550): Does this need to handle deletes? Re offline discussion: no! :)
Hi, I don't really have the background to read this so this is mostly a rubber stamp lgtm. I see that you have kept has much of the previous behavior as possible, so I'm satisfied with that.
https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... File components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc (right): https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:117: // to lazily load local data, and then react to sync data by maintain internal On 2017/01/12 22:54:07, maxbogue wrote: > maintaining Done. https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:247: // |unique_to_local_| should typically be done through ReadEntry(). On 2017/01/12 22:54:07, maxbogue wrote: > My previous comment here was wrong. What I meant to say was that > InitializeIfNeeded() must be called before unique_to_local_ is valid, right? You > should clarify that here or in the unique_to_local_ comment. Done. https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:250: // Important to note that because AutofillEntry's operator > simply compares On 2017/01/12 22:54:08, maxbogue wrote: > the most nit of nits: s/>/< Done. https://codereview.chromium.org/2624883002/diff/80001/components/autofill/cor... components/autofill/core/browser/webdata/autocomplete_sync_bridge.cc:315: // TODO(skym, crbug.com/680550): Does this need to handle deletes? On 2017/01/12 22:54:07, maxbogue wrote: > Re offline discussion: no! :) Done.
The CQ bit was checked by skym@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, maxbogue@chromium.org Link to the patchset: https://codereview.chromium.org/2624883002/#ps100001 (title: "More updates for Max's 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": 100001, "attempt_start_ts": 1484326008479120,
"parent_rev": "212e946fe9e8ae00534fa86a4b2755bf1cf4810b", "commit_rev":
"0299ea23ebf462d0bec0d9571711b8d6c437cb8c"}
Message was sent while issue was closed.
Description was changed from ========== [Sync] ApplySyncChanges autofill implementation. Updates autofill USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Don't trust the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. In an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from an AutofillSpecifics with no timestamps is kind of invalid, we end up setting our creation timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 ========== to ========== [Sync] ApplySyncChanges autofill implementation. Updates autofill USS implementation to accept incoming sync changes. The previous implementation can be found at AutocompleteSyncableService::ProcessSyncChanges, which was used to base the behavior off of for this CL. I've tried to setup the code in such a way that allows apply and merge to share as much code as possible. I tried to keep the overhead/complexity to a minimum, not quite sure how successful I have been. Some of the odd/gnarly pieces of logic that were (or not) copied over that I think may be worth calling out: 1. AutofillTable::GetAllAutofillEntries is only called if needed. While the processor shouldn't call us with no changes for ApplySyncChanges, it's up to us to avoid this when we're only receiving deletes. This is the previous behavior. 2. AutofillSpecifics should be ignored when .has_value() returns false. These are dropped on the floor with no attempt to delete. As far back as I could find, like over 5 years ago, this has been the case. No special handling exists for .has_name(), which can return false and we'll treat it as normal. This is the previous behavior. 3. Don't trust the incoming time stamps from Sync. This is a change from how AutocompleteSyncableService was implemented. In an attempt to be conservative in what we send, and liberal in what we accept, I've tried to add the ability to handle incorrectly ordered timestamps, and correct them locally. The cost of this should be negligible since typically there should be 1 or 2 timestmaps on any given remote specifics. 4. Converting from an AutofillSpecifics with no timestamps is kind of invalid, we end up setting our creation timestamp to time 0. We explicitly avoid this scenario when updating existing data, but when the sync change is the first time we've seen this data, we don't have quite as reasonable of an option, so we set creation and last use time to time 0. This is going to result in an immediate eviction by RemoveExpiredFormElements, but this the previous behavior, so I kept it. BUG=672619 Review-Url: https://codereview.chromium.org/2624883002 Cr-Commit-Position: refs/heads/master@{#443601} Committed: https://chromium.googlesource.com/chromium/src/+/0299ea23ebf462d0bec0d9571711... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/0299ea23ebf462d0bec0d9571711... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
