Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2658843004:
[Autofill] Convert wallet addresses to local autofill profiles. (Closed)
|
Created:
3 years, 10 months ago by sebsg Modified:
3 years, 10 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, rogerm+autofillwatch_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. |
DescriptionImplemented the conversion logic from Wallet addresses to local Autofill profiles:
At each sync, looks for server addresses that have not been converted to local profiles yet. Then, for each of the non-converted Wallet addresses, look for a similar local profile to merge into.
If one is found, merge into it following the existing merge logic.
If none is found, add it as a new local Autofill profile.
Also removed the assignation of an email address for Wallet addresses based on the user's account email, as it may have blocked some useful merges.
BUG=680182
Review-Url: https://codereview.chromium.org/2658843004
Cr-Commit-Position: refs/heads/master@{#447859}
Committed: https://chromium.googlesource.com/chromium/src/+/ed255a47bc7dddd9605ca08109fa396485984dc8
Patch Set 1 #Patch Set 2 : Rebase #
Total comments: 34
Patch Set 3 : Addressed Mathp's comments #
Total comments: 4
Patch Set 4 : Addressed Roger's comment #
Total comments: 3
Patch Set 5 : Change how to get server cards #Messages
Total messages: 51 (37 generated)
The CQ bit was checked by sebsg@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.
Patchset #1 (id:1) has been deleted
Patchset #2 (id:40001) has been deleted
Patchset #1 (id:20001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
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 ========== WIP [Autofill] Convert wallet addresses to local autofill profiles. BUG=680182 ========== to ========== Implemented the conversion logic from Wallet addresses to local Autofill profiles: At each sync, looks for server addresses that have not been converted to local profiles yet. Then, for each of the non-converted Wallet addresses, look for a similar local profile to merge into. If one is found, merge into it following the existing merge logic. If none is found, add it as a new local Autofill profile. Also removed the assignation of an email address for Wallet addresses based on the user's account email, as it may have blocked some useful merges. BUG=680182 ==========
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, could you please take a look? Thanks!
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...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sebsg@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.
mathp@chromium.org changed reviewers: + rogerm@chromium.org
Cool stuff. Probably worth having two sets of eyes on this, adding Roger. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:522: enum WalletAddressConversionType { enum WalletAddressConversionType : int { https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:767: bool PersonalDataManager::HasServerData() const { This seems unused. Delete? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1847: // Put the local profiles into a vector<AutofillProfile>. Theses are the Put -> Copy https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1853: for (auto* existing_profile : GetProfilesToSuggest()) { auto -> AutofillProfile? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1862: for (auto& wallet_address : server_profiles_) { avoid using auto if the type is straightforward https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1867: std::string address_guid = MergeServerAddressesIntoLocalProfiles( MergeServerAddressIntoProfiles? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1896: const AutofillProfile& converted_server_address, It's not actually converted yet. |server_address|? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1897: std::vector<AutofillProfile>* local_profiles) { I actually like |existing_profiles| https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1904: AutofillProfileComparator comparator(app_locale_); It looks like you took this from https://cs.chromium.org/chromium/src/components/autofill/core/browser/persona... Any chance of reusing the code? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1924: local_profiles->back().set_modification_date(base::Time::Now()); Are we advising people to use AutofillClock now? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:187: // TODO(687352): Remove one of these since they do the same thing. nit: TODO(crbug.com/687352) https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:188: // |GetProfiles()| and |web_profiles()| returns only web profiles. "web" profiles means nothing... https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:519: // called after all sync is done. can you be more precise with "all sync" https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5655: std::string server_address_id = "server_address1"; nit: const std::string kServerAddressId("server_address1")? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5657: // Add two different profile, a local and a server one. Set the use stats so *profiles https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5658: // the server card has a higher frecency, to have a predicatble ordering to card -> profile? *predictable https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5660: // Add a local profile. remove line
The CQ bit was checked by sebsg@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...
Thanks! Another look? https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/autofill_metrics.h (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/autofill_metrics.h:522: enum WalletAddressConversionType { On 2017/02/02 01:22:52, Mathieu Perreault wrote: > enum WalletAddressConversionType : int { Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:767: bool PersonalDataManager::HasServerData() const { On 2017/02/02 01:22:52, Mathieu Perreault wrote: > This seems unused. Delete? Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1847: // Put the local profiles into a vector<AutofillProfile>. Theses are the On 2017/02/02 01:22:52, Mathieu Perreault wrote: > Put -> Copy Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1853: for (auto* existing_profile : GetProfilesToSuggest()) { On 2017/02/02 01:22:52, Mathieu Perreault wrote: > auto -> AutofillProfile? Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1862: for (auto& wallet_address : server_profiles_) { On 2017/02/02 01:22:52, Mathieu Perreault wrote: > avoid using auto if the type is straightforward Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1867: std::string address_guid = MergeServerAddressesIntoLocalProfiles( On 2017/02/02 01:22:52, Mathieu Perreault wrote: > MergeServerAddressIntoProfiles? Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1896: const AutofillProfile& converted_server_address, On 2017/02/02 01:22:52, Mathieu Perreault wrote: > It's not actually converted yet. |server_address|? Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1897: std::vector<AutofillProfile>* local_profiles) { On 2017/02/02 01:22:52, Mathieu Perreault wrote: > I actually like |existing_profiles| Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1904: AutofillProfileComparator comparator(app_locale_); On 2017/02/02 01:22:52, Mathieu Perreault wrote: > It looks like you took this from > https://cs.chromium.org/chromium/src/components/autofill/core/browser/persona... > > Any chance of reusing the code? Yeah I thought about it for a while but the original caller will need to be modified a bit, so I prefer to do it in a different CL. I'll add a todo https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:1924: local_profiles->back().set_modification_date(base::Time::Now()); On 2017/02/02 01:22:52, Mathieu Perreault wrote: > Are we advising people to use AutofillClock now? It just became available with the Revase (Patch2) so yeah! Thanks ;) https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:187: // TODO(687352): Remove one of these since they do the same thing. On 2017/02/02 01:22:53, Mathieu Perreault wrote: > nit: TODO(crbug.com/687352) Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:188: // |GetProfiles()| and |web_profiles()| returns only web profiles. On 2017/02/02 01:22:52, Mathieu Perreault wrote: > "web" profiles means nothing... Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:519: // called after all sync is done. On 2017/02/02 01:22:52, Mathieu Perreault wrote: > can you be more precise with "all sync" Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... File components/autofill/core/browser/personal_data_manager_unittest.cc (right): https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5655: std::string server_address_id = "server_address1"; On 2017/02/02 01:22:53, Mathieu Perreault wrote: > nit: const std::string kServerAddressId("server_address1")? Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5657: // Add two different profile, a local and a server one. Set the use stats so On 2017/02/02 01:22:53, Mathieu Perreault wrote: > *profiles Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5658: // the server card has a higher frecency, to have a predicatble ordering to On 2017/02/02 01:22:53, Mathieu Perreault wrote: > card -> profile? > > *predictable Done. https://codereview.chromium.org/2658843004/diff/120001/components/autofill/co... components/autofill/core/browser/personal_data_manager_unittest.cc:5660: // Add a local profile. On 2017/02/02 01:22:53, Mathieu Perreault wrote: > remove line Done.
sebsg@chromium.org changed reviewers: + jwd@chromium.org
+ jwd@ Could you please review histograms.xml? Thanks!
https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:742: std::vector<AutofillProfile*> PersonalDataManager::server_profiles() const { why not just return a const ref to server_profiles_ This appears to only be used in test code? Or did I miss a use?
The CQ bit was checked by sebsg@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...
Thanks Roger, Another look? https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:742: std::vector<AutofillProfile*> PersonalDataManager::server_profiles() const { On 2017/02/02 15:32:23, Roger McFarlane (Chromium) wrote: > why not just return a const ref to server_profiles_ > > This appears to only be used in test code? Or did I miss a use? Done.
lgtm https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:569: bool has_synced_new_data = false; has_synced_new_data_?
https://codereview.chromium.org/2658843004/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2658843004/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:197: virtual const std::vector<CreditCard*>& GetCreditCards() const; Do you know why these are all virtual? Do they get overridden for test purposes? So, it looks like consistency is the reason for exposing vector of pointer instead of directly returning the underlying containers in web_profiles() and server_profiles(). General a getter (where the method name is just the member name without the trailing underscore doesn't do any work, it just exposes the const member. If consistency to the other vector of raw pointer returning methods is preferred, I'd change them to GetWebProfiles() and GetServerProfiles(), respectively.
lgtm
Thanks! Roger, please see my comment inline https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2658843004/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:569: bool has_synced_new_data = false; On 2017/02/02 15:52:30, Mathieu Perreault wrote: > has_synced_new_data_? Done. https://codereview.chromium.org/2658843004/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2658843004/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:197: virtual const std::vector<CreditCard*>& GetCreditCards() const; On 2017/02/02 16:05:01, Roger McFarlane (Chromium) wrote: > Do you know why these are all virtual? Do they get overridden for test purposes? > > So, it looks like consistency is the reason for exposing vector of pointer > instead of directly returning the underlying containers in web_profiles() and > server_profiles(). > > General a getter (where the method name is just the member name without the > trailing underscore doesn't do any work, it just exposes the const member. > > If consistency to the other vector of raw pointer returning methods is > preferred, I'd change them to GetWebProfiles() and GetServerProfiles(), > respectively. Yeah, they are overrriden in test_personal_data_manager. So I brought back the old implementation and return type but change the name to follow the convention. I would like to change the existing functions in another CL (already created a bug to refactor this since now we have 2 very similar functions).It will affect a couple of files and I want to take the time to check whether we could just return a const ref to the actual data. WDYT?
The CQ bit was checked by sebsg@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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
lgtm https://codereview.chromium.org/2658843004/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2658843004/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:197: virtual const std::vector<CreditCard*>& GetCreditCards() const; On 2017/02/02 16:38:27, sebsg wrote: > On 2017/02/02 16:05:01, Roger McFarlane (Chromium) wrote: > > Do you know why these are all virtual? Do they get overridden for test > purposes? > > > > So, it looks like consistency is the reason for exposing vector of pointer > > instead of directly returning the underlying containers in web_profiles() and > > server_profiles(). > > > > General a getter (where the method name is just the member name without the > > trailing underscore doesn't do any work, it just exposes the const member. > > > > If consistency to the other vector of raw pointer returning methods is > > preferred, I'd change them to GetWebProfiles() and GetServerProfiles(), > > respectively. > > Yeah, they are overrriden in test_personal_data_manager. > > So I brought back the old implementation and return type but change the name to > follow the convention. I would like to change the existing functions in another > CL (already created a bug to refactor this since now we have 2 very similar > functions).It will affect a couple of files and I want to take the time to check > whether we could just return a const ref to the actual data. WDYT? YOu can return a const ref if the function doesn't filter/combine the data and the caller doesn't attempt to modify the returned container.
Thanks! Sending to CQ
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, jwd@chromium.org Link to the patchset: https://codereview.chromium.org/2658843004/#ps180001 (title: "Change how to get server cards")
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": 180001, "attempt_start_ts": 1486070742390000,
"parent_rev": "c398506e3abc0799330e39144194d70c00e4032e", "commit_rev":
"ed255a47bc7dddd9605ca08109fa396485984dc8"}
Message was sent while issue was closed.
Description was changed from ========== Implemented the conversion logic from Wallet addresses to local Autofill profiles: At each sync, looks for server addresses that have not been converted to local profiles yet. Then, for each of the non-converted Wallet addresses, look for a similar local profile to merge into. If one is found, merge into it following the existing merge logic. If none is found, add it as a new local Autofill profile. Also removed the assignation of an email address for Wallet addresses based on the user's account email, as it may have blocked some useful merges. BUG=680182 ========== to ========== Implemented the conversion logic from Wallet addresses to local Autofill profiles: At each sync, looks for server addresses that have not been converted to local profiles yet. Then, for each of the non-converted Wallet addresses, look for a similar local profile to merge into. If one is found, merge into it following the existing merge logic. If none is found, add it as a new local Autofill profile. Also removed the assignation of an email address for Wallet addresses based on the user's account email, as it may have blocked some useful merges. BUG=680182 Review-Url: https://codereview.chromium.org/2658843004 Cr-Commit-Position: refs/heads/master@{#447859} Committed: https://chromium.googlesource.com/chromium/src/+/ed255a47bc7dddd9605ca08109fa... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001) as https://chromium.googlesource.com/chromium/src/+/ed255a47bc7dddd9605ca08109fa... |
