Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1982623002:
[Autofill] Sort profiles and credit cards by frecency in PaymentRequest. (Closed)
|
Created:
4 years, 7 months ago by sebsg Modified:
4 years, 6 months ago CC:
bondd+autofillwatch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, estade+watch_chromium.org, jdonnelly+autofillwatch_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistautofill_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSorted the profiles and credit cards to be used in PaymentRequest.
Also added test methods to set the use count and use date of profiles and cards.
BUG=609864
TEST=PersonalDataManagerTestJava
Committed: https://crrev.com/1df662834b5dec9a400f4cde1e6acdafd39fa1e2
Cr-Commit-Position: refs/heads/master@{#397562}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Addressed comments and refactored #
Total comments: 15
Patch Set 3 : Addressed mathp's comments #
Total comments: 15
Patch Set 4 : Addressed rouslan's comments #Patch Set 5 : Made separate paths for settings and suggestion data. #Patch Set 6 : Rebase #Patch Set 7 : Refactored and added tests #Patch Set 8 : Fixed calls to old get(Profiles|CreditCards) #Patch Set 9 : Removed unnecessary method #
Total comments: 22
Patch Set 10 : Addressed comments #
Total comments: 28
Patch Set 11 : Addressed comments #
Total comments: 2
Patch Set 12 : Fixed comment #Messages
Total messages: 58 (31 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== [Autofill] Sort profiles and credit cards by frecency in PaymentRequest. BUG=609864 TEST=PersonalDataManagerTestJava ========== to ========== Sorted the profiles and credit cards to be used in PaymentRequest. Also added test methods to set the use count and use date of profiles and cards. BUG=609864 TEST=PersonalDataManagerTestJava ==========
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, would you take a first look at this CL? Thanks!
https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:550: public void setProfileUseStats(String guid, int count, int date) { s/setProfileUseStats/setProfileUseStatsForTest https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:550: public void setProfileUseStats(String guid, int count, int date) { @VisibleForTesting https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:555: public void setCreditCardUseStats(String guid, int count, int date) { s/setCreditCardUseStats/setCreditCardUseStatsForTest https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:555: public void setCreditCardUseStats(String guid, int count, int date) { @VisibleForTesting https://codereview.chromium.org/1982623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:299: // The first profile has a lower use count that the two other profiles. It also has an older s/that/than https://codereview.chromium.org/1982623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:335: // The first card has a lower use count that the two other cards. It also has an older s/that/than https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:297: ScopedJavaLocalRef<jobject> PersonalDataManagerAndroid::GetProfileByIndex( GetProfileByIndex is now suddenly O(n*log(n)) because of sort instead of O(n) because of getting all profiles. Would it be sensible to remove this function entirely and use GetProfileByGUID instead? https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:304: // Rank the profiles by frecency (see AutofillDataModel for details). Can you put this block of code into a helper function in the anonymous namespace? Then you can re-use the helper below as well. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:306: std::sort(profiles.begin(), profiles.end(), #include <algorithm> https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:375: std::sort(profiles.begin(), profiles.end(), #include <algorithm> https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:404: std::sort(credit_cards.begin(), credit_cards.end(), #include <algorithm> https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:121: void SetProfileUseStats( s/SetProfileUseStats/SetProfileUseStatsForTest https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:130: void SetCreditCardUseStats( s/SetCreditCardUseStats/SetCreditCardUseStatsForTest
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:120001) has been deleted
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, could you please review the C++ files? Thanks! Rouslan, what do you think? Thanks! https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:550: public void setProfileUseStats(String guid, int count, int date) { On 2016/05/16 15:57:46, Rouslan wrote: > s/setProfileUseStats/setProfileUseStatsForTest Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:550: public void setProfileUseStats(String guid, int count, int date) { On 2016/05/16 15:57:46, Rouslan wrote: > @VisibleForTesting Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:555: public void setCreditCardUseStats(String guid, int count, int date) { On 2016/05/16 15:57:46, Rouslan wrote: > s/setCreditCardUseStats/setCreditCardUseStatsForTest Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:555: public void setCreditCardUseStats(String guid, int count, int date) { On 2016/05/16 15:57:46, Rouslan wrote: > @VisibleForTesting Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:299: // The first profile has a lower use count that the two other profiles. It also has an older On 2016/05/16 15:57:46, Rouslan wrote: > s/that/than Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:335: // The first card has a lower use count that the two other cards. It also has an older On 2016/05/16 15:57:46, Rouslan wrote: > s/that/than Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:297: ScopedJavaLocalRef<jobject> PersonalDataManagerAndroid::GetProfileByIndex( On 2016/05/16 15:57:46, Rouslan wrote: > GetProfileByIndex is now suddenly O(n*log(n)) because of sort instead of O(n) > because of getting all profiles. Would it be sensible to remove this function > entirely and use GetProfileByGUID instead? Yes that's a good idea. I added a method that returns the guids in they order they should be presented to the user. I thought of just returning a list or array of profiles/cards but it didn't seem easy to do. I suppose that's why they were pulled from their index before. If it's something you would like I could try to do it in a future CL. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:304: // Rank the profiles by frecency (see AutofillDataModel for details). On 2016/05/16 15:57:46, Rouslan wrote: > Can you put this block of code into a helper function in the anonymous > namespace? Then you can re-use the helper below as well. Even better, I made GetProfilesToSuggest and GetCreditCardsToSuggest in PersonalDataManager. These methods only return the profiles/cards that should be suggested to the user, so they are already deduped and sorted. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:306: std::sort(profiles.begin(), profiles.end(), On 2016/05/16 15:57:46, Rouslan wrote: > #include <algorithm> Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:375: std::sort(profiles.begin(), profiles.end(), On 2016/05/16 15:57:46, Rouslan wrote: > #include <algorithm> Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.cc:404: std::sort(credit_cards.begin(), credit_cards.end(), On 2016/05/16 15:57:46, Rouslan wrote: > #include <algorithm> Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:121: void SetProfileUseStats( On 2016/05/16 15:57:46, Rouslan wrote: > s/SetProfileUseStats/SetProfileUseStatsForTest Done. https://codereview.chromium.org/1982623002/diff/20001/chrome/browser/autofill... chrome/browser/autofill/android/personal_data_manager_android.h:130: void SetCreditCardUseStats( On 2016/05/16 15:57:46, Rouslan wrote: > s/SetCreditCardUseStats/SetCreditCardUseStatsForTest Done.
https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:489: String[] profileGUIDs = nativeGetProfileGUIDsToSuggest(mPersonalDataManagerAndroid); Why do we need to get the GUIDs before the profiles? Can we not have a method that returns the profiles themselves, below? nativeGetProfileByGUID -> nativeGetProfilesToSuggest Is it because we can't return an array of AutofillProfile over the JNI bridge? https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:561: public void setCreditCardUseStatsForTest(String guid, int count, int date) { nit: ForTesting https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:292: jint PersonalDataManagerAndroid::GetProfileCount( Looks like this function can be removed? If not, we should probably work to remove it. It has high complexity just to return a count. https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:307: for (AutofillProfile* profile : profiles) { nit: no need for braces here and below https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:73: // user. The cards are already deduped and sorted by frecency with the nit: I wouldn't make implementation specific comments in case it ever changes. Here you can say that the suggestions are processed and ready to be shown to the user (and perhaps "see X class for details") https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:24: #include "components/autofill/core/browser/autofill_data_util.h" needed? https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:194: // and ordered by frecency with the expired cards at the back. nit: back -> end of the list?
Thanks for your comments! https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:489: String[] profileGUIDs = nativeGetProfileGUIDsToSuggest(mPersonalDataManagerAndroid); On 2016/05/20 13:57:08, Mathieu Perreault wrote: > Why do we need to get the GUIDs before the profiles? Can we not have a method > that returns the profiles themselves, below? > > nativeGetProfileByGUID -> nativeGetProfilesToSuggest > > Is it because we can't return an array of AutofillProfile over the JNI bridge? > I was unsure myself. Here is what I asked in another comment to Rouslan: I added a method that returns the guids in the order they should be presented to the user. I thought of just returning a list or array of profiles/cards but it didn't seem easy to do. I suppose that's why they were pulled from their index before. If it's something you would like I could try to do it here or in a future CL. https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:561: public void setCreditCardUseStatsForTest(String guid, int count, int date) { On 2016/05/20 13:57:08, Mathieu Perreault wrote: > nit: ForTesting Done. https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:292: jint PersonalDataManagerAndroid::GetProfileCount( On 2016/05/20 13:57:08, Mathieu Perreault wrote: > Looks like this function can be removed? > > If not, we should probably work to remove it. It has high complexity just to > return a count. Done. https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:307: for (AutofillProfile* profile : profiles) { On 2016/05/20 13:57:08, Mathieu Perreault wrote: > nit: no need for braces here and below Done. https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/140001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:73: // user. The cards are already deduped and sorted by frecency with the On 2016/05/20 13:57:08, Mathieu Perreault wrote: > nit: I wouldn't make implementation specific comments in case it ever changes. > Here you can say that the suggestions are processed and ready to be shown to the > user (and perhaps "see X class for details") Good point. https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:24: #include "components/autofill/core/browser/autofill_data_util.h" On 2016/05/20 13:57:09, Mathieu Perreault wrote: > needed? Done. https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/1982623002/diff/140001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:194: // and ordered by frecency with the expired cards at the back. On 2016/05/20 13:57:09, Mathieu Perreault wrote: > nit: back -> end of the list? Done.
lgtm https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:489: String[] profileGUIDs = nativeGetProfileGUIDsToSuggest(mPersonalDataManagerAndroid); On 2016/05/24 at 17:27:32, sebsg wrote: > On 2016/05/20 13:57:08, Mathieu Perreault wrote: > > Why do we need to get the GUIDs before the profiles? Can we not have a method > > that returns the profiles themselves, below? > > > > nativeGetProfileByGUID -> nativeGetProfilesToSuggest > > > > Is it because we can't return an array of AutofillProfile over the JNI bridge? > > > > I was unsure myself. Here is what I asked in another comment to Rouslan: > > I added a method that returns the guids in the order > they should be presented to the user. I thought of just returning a list or > array of profiles/cards but it didn't seem easy to do. I suppose that's why they > were pulled from their index before. If it's something you would like I could > try to do it here or in a future CL. It does seem difficult. I'm fine with this solution.
https://codereview.chromium.org/1982623002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/1982623002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:126: void setProfileUseStatsForTesting(final String guid, final int count, final int date) Please document the format for |date| and what will happen if I pass in negative |count| or |date|. For example, say "count and date should be non-negative" and then add "assert count >= 0 && date >= 0;" line in PersonalDataManager. This will trigger only in debug builds. https://codereview.chromium.org/1982623002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:137: void setCreditCardUseStatsForTesting(final String guid, final int count, final int date) Ditto. https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:468: size_t count_size_t = static_cast<size_t>(count); You use this variable only once, so it's OK to inline it. https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:485: size_t count_size_t = static_cast<size_t>(count); Inline https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:109: // Sets the use count and use date of the profile associated to the |jguid|. Ditto about |count| and |date| range assumptions and |date| format. https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:119: void SetCreditCardUseStatsForTest( Ditto. https://codereview.chromium.org/1982623002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1982623002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:810: DedupeCreditCardToSuggest(&cards_to_dedupe); Can you check that this code path is not being used in for displaying cards in chrome://settings/autofill and in the Android equivalent? Deduplication is good for suggestions, but should not be happening in settings. Settings should reflect exactly what's no disk, so the user can manager their own data. For example, PersonalDataManager::getProfiles() (in Java) is used by Android Autofill settings. We would not want deduplication there, if it was being used for settings. If you find that settings is performing deduping, then we would need two separate code paths: one for settings without dedupe and one for PaymentRequest/autofill with dedupe.
Thanks for the comments! https://codereview.chromium.org/1982623002/diff/160001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/1982623002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:126: void setProfileUseStatsForTesting(final String guid, final int count, final int date) On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > Please document the format for |date| and what will happen if I pass in negative > |count| or |date|. For example, say "count and date should be non-negative" and > then add "assert count >= 0 && date >= 0;" line in PersonalDataManager. This > will trigger only in debug builds. Done. https://codereview.chromium.org/1982623002/diff/160001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:137: void setCreditCardUseStatsForTesting(final String guid, final int count, final int date) On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > Ditto. Done. https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.cc (right): https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:468: size_t count_size_t = static_cast<size_t>(count); On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > You use this variable only once, so it's OK to inline it. Done. https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.cc:485: size_t count_size_t = static_cast<size_t>(count); On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > Inline Done. https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:109: // Sets the use count and use date of the profile associated to the |jguid|. On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > Ditto about |count| and |date| range assumptions and |date| format. Done. https://codereview.chromium.org/1982623002/diff/160001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:119: void SetCreditCardUseStatsForTest( On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > Ditto. Done. https://codereview.chromium.org/1982623002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1982623002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:810: DedupeCreditCardToSuggest(&cards_to_dedupe); On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > Can you check that this code path is not being used in for displaying cards in > chrome://settings/autofill and in the Android equivalent? Deduplication is good > for suggestions, but should not be happening in settings. Settings should > reflect exactly what's no disk, so the user can manager their own data. > > For example, PersonalDataManager::getProfiles() (in Java) is used by Android > Autofill settings. We would not want deduplication there, if it was being used > for settings. > > If you find that settings is performing deduping, then we would need two > separate code paths: one for settings without dedupe and one for > PaymentRequest/autofill with dedupe. The options page uses GetProfiles and GetCreditCards directly. These calls just get the data without any processing. Dedupe was only called in GetCreditCardSuggestions and now in GetCreditCardsToSuggest. When you get back we could also check if you could use our suggestions directly.
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
https://codereview.chromium.org/1982623002/diff/160001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.cc (right): https://codereview.chromium.org/1982623002/diff/160001/components/autofill/co... components/autofill/core/browser/personal_data_manager.cc:810: DedupeCreditCardToSuggest(&cards_to_dedupe); On 2016/05/24 21:41:22, sebsg wrote: > On 2016/05/24 20:21:48, Rouslan (slow till June 10) wrote: > > Can you check that this code path is not being used in for displaying cards in > > chrome://settings/autofill and in the Android equivalent? Deduplication is > good > > for suggestions, but should not be happening in settings. Settings should > > reflect exactly what's no disk, so the user can manager their own data. > > > > For example, PersonalDataManager::getProfiles() (in Java) is used by Android > > Autofill settings. We would not want deduplication there, if it was being used > > for settings. > > > > If you find that settings is performing deduping, then we would need two > > separate code paths: one for settings without dedupe and one for > > PaymentRequest/autofill with dedupe. > > The options page uses GetProfiles and GetCreditCards directly. These calls just > get the data without any processing. Dedupe was only called in > GetCreditCardSuggestions and now in GetCreditCardsToSuggest. > > When you get back we could also check if you could use our suggestions directly. What about getProfiles() and getCreditCards() in https://codereview.chromium.org/1982623002/diff/220001/chrome/android/java/sr... ? That seems to be going through your new code path with de-dupe. I would suggest that PersonalDataManager.java should have getProfilesForSettings(), getProfilesToSuggest(), getCreditCardsForSettings(), and getCreditCardsToSuggest(). The *ForSettings() variants should not have deduplication. The *ToSuggest() variants should have deduplication and frecency ordering.
Patchset #6 (id:260001) has been deleted
Patchset #6 (id:280001) has been deleted
Patchset #7 (id:320001) has been deleted
Patchset #7 (id:340001) has been deleted
Patchset #8 (id:380001) has been deleted
Patchset #6 (id:300001) has been deleted
Patchset #7 (id:400001) has been deleted
Hi Rouslan could you please take another look? Thanks!
Patchset #10 (id:270013) has been deleted
Looking. What could be causing this strange error on linux_android_rel_ng during run-time? java.util.concurrent.ExecutionException: java.lang.NoSuchMethodError: org.chromium.chrome.browser.autofill.PersonalDataManager.getProfilesToSuggest
Great job! This is good progress and we're moving in the right direction. Overall, I see that you're trying to write very generic and reusable code, which is a good software engineering practice. It's a bit hard to keep all of this code in my head at once, so this may not be the final review iteration. Don't be discouraged by that, however. Please see my comments inline. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:480: public List<AutofillProfile> getProfilesToSuggest() { This method is never called in production. Let's remove it. If you have any tests for getProfilesToSuggest(), use getAddressOnlyProfilesToSuggest() in there instead. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:488: private List<AutofillProfile> getProfilesWithAddressOnlyLabelsForSettings(boolean addressOnly) { This function is used only in getProfilesForSettings(), so you can merge it into getProfilesForSettings(). As a corollary, |addressOnly| is always false here, so there's no need for the function parameter. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:492: nativeGetProfileGUIDsForSettings(mPersonalDataManagerAndroid)); Java-to-Native and Native-to-Java calls are relatively expensive. Can you figure out a way to combine nativeGetProfileLabelsForSettings() and nativeGetProfileGUIDsForSettings() into a single function? https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:495: private List<AutofillProfile> getProfilesWithAddressOnlyLabelsToSuggest(boolean addressOnly) { After getProfilesToSuggest() is removed, this method only called from getAddressOnlyProfilesToSuggest(). So you can merge this method into getAddressOnlyProfilesToSuggest(). As a corollary, |addressOnly| is always true here, so there's no need for the function parameter. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:150: // more details see the comment header in time.h. Let's use jsdoc style comments in Java: /** * General comment here. * @param guid Describe guid here. * @param count Describe count here. * @param date Describe date here. */ https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:165: void setCreditCardUseStatsForTesting(final String guid, final int count, final int date) jsdoc here as well. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:45: assertEquals(1, mHelper.getNumberOfProfilesToSuggest()); *ToSuggest() variants of the functions are new and are tested in the new tests below: testProfilesFrecency(), testCreditCardsFrecency(), and testCreditCardsDeduping(). *ForSettings() variants have the same behavior as the old functions. Therefore, let's use *ForSettings() variants everywhere except your new tests. https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:63: bool address_only); This param will be always false, so there's no need for it. I was using |address_only| boolean to distinguish between settings and PaymentRequest needs. Now that we have two separate methods, we can remove |address_only| parameter. It's always true for PaymentRequest (ToSuggest) and always false for settings. https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:75: bool address_only); This param will be always true, so there's no need for it. https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:83: const base::android::JavaParamRef<jobject>& unused_ob); s/unused_ob/unused_obj/ https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:89: const base::android::JavaParamRef<jobject>& unused_ob); s/unused_ob/unused_obj/
Thanks for the comments, the code is getting better. Could you take another look? Thanks! https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:480: public List<AutofillProfile> getProfilesToSuggest() { On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > This method is never called in production. Let's remove it. If you have any > tests for getProfilesToSuggest(), use getAddressOnlyProfilesToSuggest() in there > instead. Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:488: private List<AutofillProfile> getProfilesWithAddressOnlyLabelsForSettings(boolean addressOnly) { On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > This function is used only in getProfilesForSettings(), so you can merge it into > getProfilesForSettings(). > > As a corollary, |addressOnly| is always false here, so there's no need for the > function parameter. Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:492: nativeGetProfileGUIDsForSettings(mPersonalDataManagerAndroid)); On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > Java-to-Native and Native-to-Java calls are relatively expensive. Can you figure > out a way to combine nativeGetProfileLabelsForSettings() and > nativeGetProfileGUIDsForSettings() into a single function? I added a TODO and I'm going to tackle this issue right after this CL. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:495: private List<AutofillProfile> getProfilesWithAddressOnlyLabelsToSuggest(boolean addressOnly) { On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > After getProfilesToSuggest() is removed, this method only called from > getAddressOnlyProfilesToSuggest(). So you can merge this method into > getAddressOnlyProfilesToSuggest(). > > As a corollary, |addressOnly| is always true here, so there's no need for the > function parameter. Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:150: // more details see the comment header in time.h. On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > Let's use jsdoc style comments in Java: > > /** > * General comment here. > * @param guid Describe guid here. > * @param count Describe count here. > * @param date Describe date here. > */ Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:165: void setCreditCardUseStatsForTesting(final String guid, final int count, final int date) On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > jsdoc here as well. Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:45: assertEquals(1, mHelper.getNumberOfProfilesToSuggest()); On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > *ToSuggest() variants of the functions are new and are tested in the new tests > below: testProfilesFrecency(), testCreditCardsFrecency(), and > testCreditCardsDeduping(). > > *ForSettings() variants have the same behavior as the old functions. > > Therefore, let's use *ForSettings() variants everywhere except your new tests. Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:63: bool address_only); On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > This param will be always false, so there's no need for it. > > I was using |address_only| boolean to distinguish between settings and > PaymentRequest needs. Now that we have two separate methods, we can remove > |address_only| parameter. It's always true for PaymentRequest (ToSuggest) and > always false for settings. Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:75: bool address_only); On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > This param will be always true, so there's no need for it. Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:83: const base::android::JavaParamRef<jobject>& unused_ob); On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > s/unused_ob/unused_obj/ Done. https://codereview.chromium.org/1982623002/diff/460001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:89: const base::android::JavaParamRef<jobject>& unused_ob); On 2016/05/27 21:47:30, Rouslan (ツ) wrote: > s/unused_ob/unused_obj/ Done.
rouslan@chromium.org changed reviewers: + dfalcantara@chromium.org
lgtm % nits +dfalcantara@ for Java owner review. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:520: nit: no new line. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:526: nit: no new line. https://codereview.chromium.org/1982623002/diff/480001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/480001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:66: // Fields such as phone number and email address are also omitted, but all This comment is inconsistent. Above you say "at least 2 fields." Here you say "all fields are included." I think the latter is correct. Please clean up.
https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:477: public List<AutofillProfile> getProfilesForSettings() { Add a javadoc here so that people who aren't familiar with Payments know why it is different from the similarly named one below. Given their similarities, is there any reason you switched away from using the boolean? Seems like you could've used something like an enum to make this clearer without making multiple similarly-named flavors of the same method. https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:483: // TODO(crbug.com/616102): Reduce the number of Java to Native calls when getting profiles. This really is kind of an absurd number of JNI calls, but it seems that the number of calls hasn't increased, overall. Is there a plan for this? https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:572: public void setProfileUseStatsForTesting(String guid, int count, int date) { 1) Can this be package protected? 2) Timestamps are longs, not ints. https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:55: int getNumberOfProfilesToSuggest() throws ExecutionException { Does it make sense to factor out the bit that fires a Callable to getProfilesToSuggest, have this one call that method, then just return its size? It could significantly reduce the duplication here. https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:183: * @param date The use date to assign to the profile. It represents an absolute point in For multiline javadocs, it's preferable to indent every line after the first to the beginning of the first line's comment. @param date The use date to assign to the profile. It represents an absolute point in coordinated universal time (UTC) represented as microseconds since the etc etc https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:302: // use date that the second profile and the same use date as the Wrap correctly at 100. https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:350: List<CreditCard> cards = mHelper.getCreditCardsToSuggest(); nit: no newline? https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:372: // Only one card should be suggested to the user since the two are identical . nit: no space before the period. https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/browser/... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/browser/... chrome/browser/autofill/android/personal_data_manager_android.h:169: // Gets the labels for the |profiles| passed as paremeters. These labels are nit: parameters https://chromiumcodereview.appspot.com/1982623002/diff/480001/components/auto... File components/autofill/core/browser/personal_data_manager.h (right): https://chromiumcodereview.appspot.com/1982623002/diff/480001/components/auto... components/autofill/core/browser/personal_data_manager.h:196: // Returns the credit cards to suggest to the users. Those have been deduped nit: Be consistent about the sentence target. You use "to the user" above and "to the users" here.
Patchset #11 (id:500001) has been deleted
https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:477: public List<AutofillProfile> getProfilesForSettings() { On 2016/05/31 22:39:42, dfalcantara wrote: > Given their similarities, is > there any reason you switched away from using the boolean? Seems like you > could've used something like an enum to make this clearer without making > multiple similarly-named flavors of the same method. It was originally my request. getProfiles(), which is now called getProfilesForSettings() is called in multiple places, so I wanted to make sure those places don't use the wrong version accidentally. Both function name and enum parameter provide more verbosity than a boolean that was here before. I'm OK with using an enum, if you would prefer that.
https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:477: public List<AutofillProfile> getProfilesForSettings() { On 2016/06/01 17:42:59, Rouslan (ツ) wrote: > On 2016/05/31 22:39:42, dfalcantara wrote: > > Given their similarities, is > > there any reason you switched away from using the boolean? Seems like you > > could've used something like an enum to make this clearer without making > > multiple similarly-named flavors of the same method. > > It was originally my request. getProfiles(), which is now called > getProfilesForSettings() is called in multiple places, so I wanted to make sure > those places don't use the wrong version accidentally. Both function name and > enum parameter provide more verbosity than a boolean that was here before. I'm > OK with using an enum, if you would prefer that. Eh, it's fine. I'm just worried that you could get into a situation where you have to add another flavor of these methods and the code becomes harder to read. I'll defer to your judgement.
Patchset #12 (id:540001) has been deleted
Patchset #11 (id:520001) has been deleted
Patchset #11 (id:560001) has been deleted
On 2016/06/01 17:48:10, dfalcantara wrote: > https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... > File > chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java > (right): > > https://chromiumcodereview.appspot.com/1982623002/diff/480001/chrome/android/... > chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:477: > public List<AutofillProfile> getProfilesForSettings() { > On 2016/06/01 17:42:59, Rouslan (ツ) wrote: > > On 2016/05/31 22:39:42, dfalcantara wrote: > > > Given their similarities, is > > > there any reason you switched away from using the boolean? Seems like you > > > could've used something like an enum to make this clearer without making > > > multiple similarly-named flavors of the same method. > > > > It was originally my request. getProfiles(), which is now called > > getProfilesForSettings() is called in multiple places, so I wanted to make > sure > > those places don't use the wrong version accidentally. Both function name and > > enum parameter provide more verbosity than a boolean that was here before. I'm > > OK with using an enum, if you would prefer that. > > Eh, it's fine. I'm just worried that you could get into a situation where you > have to add another flavor of these methods and the code becomes harder to read. > I'll defer to your judgement. Enum sounds good. sebsg@: Enums in Android+JNI are somewhat tricky. Let me know if you can't find any usable examples already in code.
Patchset #11 (id:580001) has been deleted
Thanks for the comments. Could you take another look? Thanks! https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:477: public List<AutofillProfile> getProfilesForSettings() { On 2016/05/31 22:39:42, dfalcantara wrote: > Add a javadoc here so that people who aren't familiar with Payments know why it > is different from the similarly named one below. Given their similarities, is > there any reason you switched away from using the boolean? Seems like you > could've used something like an enum to make this clearer without making > multiple similarly-named flavors of the same method. Good point to the javadoc. I'll stay purposely vague because we are actively modifying the processing in the *ToSuggest path. We currently sort the data by Frecency (frequency and recency) and dedupe them. For the naming, I thought it would be clearer that way because they follow a linear path down to a native method with the (almost) same name. For example, getProfilesToSuggest in PersonalDataManager.java ends up calling GetProfileToSuggest in personal_data_manager.cc. Almost because get(Profiles|CreditCards)ToSuggest end up calling get(Profiles|CreditCards) in the native code. I can change it to an enum if you prefer. I prefer this way but I can see why you would prefer the other way. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:483: // TODO(crbug.com/616102): Reduce the number of Java to Native calls when getting profiles. On 2016/05/31 22:39:42, dfalcantara wrote: > This really is kind of an absurd number of JNI calls, but it seems that the > number of calls hasn't increased, overall. Is there a plan for this? You are correct, this seems like an unncessary number of calls but it's the same number as it was before this CL. I'd like to work on that as a next CL. I created un bug for this (crbug.com/616102). https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:520: On 2016/05/31 18:51:56, Rouslan (ツ) wrote: > nit: no new line. Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:526: On 2016/05/31 18:51:56, Rouslan (ツ) wrote: > nit: no new line. Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:572: public void setProfileUseStatsForTesting(String guid, int count, int date) { On 2016/05/31 22:39:42, dfalcantara wrote: > 1) Can this be package protected? > 2) Timestamps are longs, not ints. Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/1982623002/diff/480001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:55: int getNumberOfProfilesToSuggest() throws ExecutionException { On 2016/05/31 22:39:42, dfalcantara wrote: > Does it make sense to factor out the bit that fires a Callable to > getProfilesToSuggest, have this one call that method, then just return its size? > It could significantly reduce the duplication here. Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:183: * @param date The use date to assign to the profile. It represents an absolute point in On 2016/05/31 22:39:42, dfalcantara wrote: > For multiline javadocs, it's preferable to indent every line after the first to > the beginning of the first line's comment. > > @param date The use date to assign to the profile. It represents an absolute > point in > coordinated universal time (UTC) represented as microseconds since > the > etc etc Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java (right): https://codereview.chromium.org/1982623002/diff/480001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:302: // use date that the second profile and the same use date as the On 2016/05/31 22:39:42, dfalcantara wrote: > Wrap correctly at 100. Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:350: List<CreditCard> cards = mHelper.getCreditCardsToSuggest(); On 2016/05/31 22:39:42, dfalcantara wrote: > nit: no newline? Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/PersonalDataManagerTest.java:372: // Only one card should be suggested to the user since the two are identical . On 2016/05/31 22:39:42, dfalcantara wrote: > nit: no space before the period. Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/browser/autofil... File chrome/browser/autofill/android/personal_data_manager_android.h (right): https://codereview.chromium.org/1982623002/diff/480001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:66: // Fields such as phone number and email address are also omitted, but all On 2016/05/31 18:51:56, Rouslan (ツ) wrote: > This comment is inconsistent. Above you say "at least 2 fields." Here you say > "all fields are included." I think the latter is correct. Please clean up. Done. https://codereview.chromium.org/1982623002/diff/480001/chrome/browser/autofil... chrome/browser/autofill/android/personal_data_manager_android.h:169: // Gets the labels for the |profiles| passed as paremeters. These labels are On 2016/05/31 22:39:42, dfalcantara wrote: > nit: parameters Done. https://codereview.chromium.org/1982623002/diff/480001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/1982623002/diff/480001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:196: // Returns the credit cards to suggest to the users. Those have been deduped On 2016/05/31 22:39:42, dfalcantara wrote: > nit: Be consistent about the sentence target. You use "to the user" above and > "to the users" here. Done.
lgtm % the nit correction https://codereview.chromium.org/1982623002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/1982623002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:158: * coordinated universal time (UTC) represented as microseconds since the Windows epoch. Er, I meant to indent to the first word after the parameter, like in the example I gave you.
Thanks everyone for your time reviewing this CL. https://codereview.chromium.org/1982623002/diff/600001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java (right): https://codereview.chromium.org/1982623002/diff/600001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/autofill/AutofillTestHelper.java:158: * coordinated universal time (UTC) represented as microseconds since the Windows epoch. On 2016/06/02 17:26:42, dfalcantara wrote: > Er, I meant to indent to the first word after the parameter, like in the example > I gave you. Oh I see, sorry my bad. The formatting was weird in my code review page and I made the wrong deduction.
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, rouslan@chromium.org, dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1982623002/#ps620001 (title: "Fixed comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982623002/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982623002/620001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rouslan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1982623002/620001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1982623002/620001
Message was sent while issue was closed.
Description was changed from ========== Sorted the profiles and credit cards to be used in PaymentRequest. Also added test methods to set the use count and use date of profiles and cards. BUG=609864 TEST=PersonalDataManagerTestJava ========== to ========== Sorted the profiles and credit cards to be used in PaymentRequest. Also added test methods to set the use count and use date of profiles and cards. BUG=609864 TEST=PersonalDataManagerTestJava ==========
Message was sent while issue was closed.
Committed patchset #12 (id:620001)
Message was sent while issue was closed.
Description was changed from ========== Sorted the profiles and credit cards to be used in PaymentRequest. Also added test methods to set the use count and use date of profiles and cards. BUG=609864 TEST=PersonalDataManagerTestJava ========== to ========== Sorted the profiles and credit cards to be used in PaymentRequest. Also added test methods to set the use count and use date of profiles and cards. BUG=609864 TEST=PersonalDataManagerTestJava Committed: https://crrev.com/1df662834b5dec9a400f4cde1e6acdafd39fa1e2 Cr-Commit-Position: refs/heads/master@{#397562} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/1df662834b5dec9a400f4cde1e6acdafd39fa1e2 Cr-Commit-Position: refs/heads/master@{#397562}
Message was sent while issue was closed.
A revert of this CL (patchset #12 id:620001) has been created in https://codereview.chromium.org/2033323003/ by johnme@chromium.org. The reason for reverting is: This broke the downstream clang-clankium-tot-builder, since PersonalDataManager at least is used by downstream autofill code to get profiles and credit cards: https://goto.google.com/kneae. |
