Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2120973002:
[Payments] Show complete profiles first and limit to 4 suggestions. (Closed)
|
Created:
4 years, 5 months ago by sebsg Modified:
4 years, 5 months ago CC:
chromium-reviews, rouslan+autofill_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSuggest complete addresses and contact details first and limit to the top four suggestions.
BUG=625565
TEST=PaymentRequestDynamicShippingMultipleAddressesTest,
PaymentRequestMultipleContactDetailTest
Committed: https://crrev.com/40dd55f9afba8922e99f8e773ce6fbb980f42196
Cr-Commit-Position: refs/heads/master@{#405302}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Patch 2 #Patch Set 3 : Rebase #Patch Set 4 : Truncate to top 4 suggestions #
Total comments: 26
Patch Set 5 : Addressed comments #
Total comments: 22
Patch Set 6 : Addressed comments #
Total comments: 8
Patch Set 7 : Addressed comments #
Total comments: 2
Patch Set 8 : Removed unnecessary notifications for use stats and addressed comments #Patch Set 9 : Added comment #
Total comments: 2
Patch Set 10 : Removed ids for suggestion labels #
Total comments: 4
Patch Set 11 : Replaced the array with a list #
Total comments: 2
Patch Set 12 : Removed the null return value #Messages
Total messages: 43 (19 generated)
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi rouslan, this is my first draft. Feel free to comment.
https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:315: static class AutofillProfileComparator implements Comparator<AutofillProfile> should be private should be right under getProfilesToSuggest. { should not be on newline in java. https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:320: boolean aIsComplete = addressEditor.isProfileComplete(a); Move isProfileComplete() somewhere to browser/autofill/.
https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:566: Collections.sort(profilesToSuggest, new AutofillProfileComparator()); Sort the shipping addresses in PaymentRequestImpl instead, because shipping addresses have different notion of "completeness" from contact info, which also uses getProfilesToSuggest() as a source of data.
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
Description was changed from
==========
[Payments] Show complete profiles first and limit to 4 suggestions.
BUG=625565
TEST=PersonalDataManagerTest
==========
to
==========
Suggest complete addresses and contact details first and limit to the top four
suggestions.
BUG=625565
TEST=PaymentRequestDynamicShippingMultipleAddressesTest,
PaymentRequestMultipleContactDetailTest
==========
Patchset #4 (id:140001) has been deleted
Hi Rouslan, could you please take a look? Thanks! https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java (right): https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:315: static class AutofillProfileComparator implements Comparator<AutofillProfile> On 2016/07/04 13:48:28, Rouslan (ツ) wrote: > should be private > > should be right under getProfilesToSuggest. > > { should not be on newline in java. Done. https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:320: boolean aIsComplete = addressEditor.isProfileComplete(a); On 2016/07/04 13:48:28, Rouslan (ツ) wrote: > Move isProfileComplete() somewhere to browser/autofill/. Done. https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:320: boolean aIsComplete = addressEditor.isProfileComplete(a); On 2016/07/04 13:48:28, Rouslan (ツ) wrote: > Move isProfileComplete() somewhere to browser/autofill/. Done. https://codereview.chromium.org/2120973002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/autofill/PersonalDataManager.java:566: Collections.sort(profilesToSuggest, new AutofillProfileComparator()); On 2016/07/04 14:01:17, Rouslan (ツ) wrote: > Sort the shipping addresses in PaymentRequestImpl instead, because shipping > addresses have different notion of "completeness" from contact info, which also > uses getProfilesToSuggest() as a source of data. Done.
Patchset #4 (id:160001) has been deleted
https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:245: if (a.isComplete() == b.isComplete()) { Make AutofillAddress and AutofillContact implement a new interface, let's call it Completable. public interface Completable { boolean isComplete(); } Then you have a common comparator for addresses and contacts. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:256: List<AutofillAddress> topAddresses = new ArrayList<AutofillAddress>( addresses = addresses.subList(0, Math.min(addresses.size(), SUGGESTIONS_LIMIT)); https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:262: (!mMerchantNeedsShippingAddress && topAddresses.size() > 0 I think "!addresses.isEmpty()" is more canonical than "addresses.size() > 0". https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:302: if (a.isComplete() == b.isComplete()) { Would this be considered bad? ;-) return (a.isComplete() ? 1 : 0) - (b.isComplete() ? 1 : 0); https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:313: List<AutofillContact> topContacts = new ArrayList<AutofillContact>( contacts = contacts.subList(...); https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:317: int firstCompleteContactIndex = (topContacts.size() > 0 !contacts.isEmpty() https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:906: currentOptionRow.setLabelId(OPTION_LABEL_IDS[i]); if (i < OPTION_LABEL_IDS.length) currentOptionRow.setLabelId(OPTION_LABEL_IDS[i]); https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java:19: * and user that has multiple addresses stored in autofill settings. Replace "multiple" with "5", because that's critical for you test. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java:34: "Homer Simpson", "Acme Inc.", "123 Main", "California", "Los Angeles", "", Please add the profiles in alphabetic order, so that the non-alphabetic order in the test case below makes your purpose clear. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java:36: String guid1 = helper.setProfile(profile1); No need to create "profileX" variables that you're going to use right away. String guid1 = helper.setProfile(new AutofillProfile(...)); https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMultipleContactDetailsTest.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMultipleContactDetailsTest.java:19: * multiple contact detail options. Replace "multiple" with "5". https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:227: if (mUI.getContactDetailsSectionForTest().findViewById( Don't call findByView() twice. Save the result and reuse it. https://codereview.chromium.org/2120973002/diff/180001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2120973002/diff/180001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:160: // Notifies observers that personal data has changed for tests. // Notifies test observers that personal data has changed.
Patchset #5 (id:200001) has been deleted
Patchset #5 (id:220001) has been deleted
Could you please take another look? Thanks! https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:245: if (a.isComplete() == b.isComplete()) { On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > Make AutofillAddress and AutofillContact implement a new interface, let's call > it Completable. > > public interface Completable { > boolean isComplete(); > } > > Then you have a common comparator for addresses and contacts. Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:256: List<AutofillAddress> topAddresses = new ArrayList<AutofillAddress>( On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > addresses = addresses.subList(0, Math.min(addresses.size(), SUGGESTIONS_LIMIT)); Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:262: (!mMerchantNeedsShippingAddress && topAddresses.size() > 0 On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > I think "!addresses.isEmpty()" is more canonical than "addresses.size() > 0". Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:302: if (a.isComplete() == b.isComplete()) { On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > Would this be considered bad? ;-) > > return (a.isComplete() ? 1 : 0) - (b.isComplete() ? 1 : 0); I prefer the current version but I'll defer decision to you :) https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:313: List<AutofillContact> topContacts = new ArrayList<AutofillContact>( On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > contacts = contacts.subList(...); Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:317: int firstCompleteContactIndex = (topContacts.size() > 0 On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > !contacts.isEmpty() Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:906: currentOptionRow.setLabelId(OPTION_LABEL_IDS[i]); On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > if (i < OPTION_LABEL_IDS.length) > currentOptionRow.setLabelId(OPTION_LABEL_IDS[i]); Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java:19: * and user that has multiple addresses stored in autofill settings. On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > Replace "multiple" with "5", because that's critical for you test. Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java:34: "Homer Simpson", "Acme Inc.", "123 Main", "California", "Los Angeles", "", On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > Please add the profiles in alphabetic order, so that the non-alphabetic order in > the test case below makes your purpose clear. Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestDynamicShippingMultipleAddressesTest.java:36: String guid1 = helper.setProfile(profile1); On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > No need to create "profileX" variables that you're going to use right away. > > String guid1 = helper.setProfile(new AutofillProfile(...)); Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMultipleContactDetailsTest.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestMultipleContactDetailsTest.java:19: * multiple contact detail options. On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > Replace "multiple" with "5". Done. https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/180001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:227: if (mUI.getContactDetailsSectionForTest().findViewById( On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > Don't call findByView() twice. Save the result and reuse it. Done. https://codereview.chromium.org/2120973002/diff/180001/components/autofill/co... File components/autofill/core/browser/personal_data_manager.h (right): https://codereview.chromium.org/2120973002/diff/180001/components/autofill/co... components/autofill/core/browser/personal_data_manager.h:160: // Notifies observers that personal data has changed for tests. On 2016/07/07 13:59:36, Rouslan (ツ) wrote: > // Notifies test observers that personal data has changed. Done.
https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:21: */ Put "implements Completable" inside of PaymentOption.java. This will allow for more code sharing. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:35: private boolean mIsComplete; Make "protected" and put in PaymentOption.java https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:49: /** @return Whether the data is complete and can be sent to the merchant as-is. */ Replace the comments with @Override. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:50: public boolean isComplete() { Put in PaymentOption.java https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java:8: * The interface for completeable types. The interface for types that may be complete, i.e., can be sent to the merchant as-is, without being edited by the user first. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java:11: // Returns whether the object is complete. Use "/**" style comments. Interface methods need JavaDoc. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:265: && addresses.get(0).isComplete()) { Should be +8. not +12 indent. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:223: throws ExecutionException { assert suggestionIndex < mOoptionLabelIds.length; https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:230: if (view == null) { return view == null ? null : ((TextView) view).getText().toString(); https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:243: throws ExecutionException { assert suggestionIndex < mOoptionLabelIds.length; https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:250: if (view == null) { Ditto.
Patchset #6 (id:260001) has been deleted
Thanks for the quick review. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:21: */ On 2016/07/08 08:36:30, Rouslan (ツ) wrote: > Put "implements Completable" inside of PaymentOption.java. This will allow for > more code sharing. Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:35: private boolean mIsComplete; On 2016/07/08 08:36:30, Rouslan (ツ) wrote: > Make "protected" and put in PaymentOption.java Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:49: /** @return Whether the data is complete and can be sent to the merchant as-is. */ On 2016/07/08 08:36:30, Rouslan (ツ) wrote: > Replace the comments with @Override. I removed the method here because it is now public in PaymentOption. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillAddress.java:50: public boolean isComplete() { On 2016/07/08 08:36:30, Rouslan (ツ) wrote: > Put in PaymentOption.java Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java:8: * The interface for completeable types. On 2016/07/08 08:36:30, Rouslan (ツ) wrote: > The interface for types that may be complete, i.e., can be sent to the merchant > as-is, without being edited by the user first. Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java:11: // Returns whether the object is complete. On 2016/07/08 08:36:30, Rouslan (ツ) wrote: > Use "/**" style comments. Interface methods need JavaDoc. Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:265: && addresses.get(0).isComplete()) { On 2016/07/08 08:36:31, Rouslan (ツ) wrote: > Should be +8. not +12 indent. Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:223: throws ExecutionException { On 2016/07/08 08:36:31, Rouslan (ツ) wrote: > assert suggestionIndex < mOoptionLabelIds.length; Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:230: if (view == null) { On 2016/07/08 08:36:31, Rouslan (ツ) wrote: > return view == null ? null : ((TextView) view).getText().toString(); Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:243: throws ExecutionException { On 2016/07/08 08:36:31, Rouslan (ツ) wrote: > assert suggestionIndex < mOoptionLabelIds.length; Done. https://codereview.chromium.org/2120973002/diff/240001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:250: if (view == null) { On 2016/07/08 08:36:31, Rouslan (ツ) wrote: > Ditto. Done.
Pretty close! https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/re... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/re... chrome/android/java/res/values/ids.xml:71: nit: no newline. https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:41: /** @return Whether the data is complete and can be sent to the merchant as-is */ Replace the comment with @Override https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:109: /** An array mapping the index of a option label to its id. */ "an option label" https://codereview.chromium.org/2120973002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:63: /** An array mapping the index of a option label to its id. */ "an option label"
Thanks PTAL? https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/re... File chrome/android/java/res/values/ids.xml (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/re... chrome/android/java/res/values/ids.xml:71: On 2016/07/08 09:36:26, Rouslan (ツ) wrote: > nit: no newline. Done. https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentOption.java:41: /** @return Whether the data is complete and can be sent to the merchant as-is */ On 2016/07/08 09:36:26, Rouslan (ツ) wrote: > Replace the comment with @Override Done. https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:109: /** An array mapping the index of a option label to its id. */ On 2016/07/08 09:36:26, Rouslan (ツ) wrote: > "an option label" Done. https://codereview.chromium.org/2120973002/diff/280001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/280001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:63: /** An array mapping the index of a option label to its id. */ On 2016/07/08 09:36:26, Rouslan (ツ) wrote: > "an option label" Done.
lgtm % nits https://codereview.chromium.org/2120973002/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java (right): https://codereview.chromium.org/2120973002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java:5: package org.chromium.chrome.browser.payments; nit: Let's put this in rg.chromium.chrome.browser.payments.ui package, because the only implementor is PaymentOption.java, which is in org.chromium.chrome.browser.payments.ui package. https://codereview.chromium.org/2120973002/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/Completable.java:12: /** @return Whether the data is complete and can be sent to the merchant as-is */ nit: only one space between * and @.
sebsg@chromium.org changed reviewers: + bauerb@chromium.org
Hi bauerb, could you please take a look?
https://codereview.chromium.org/2120973002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:906: if (i < OPTION_LABEL_IDS.length) { Having to reserve ID resources for these labels just for testing seems a bit involved. You could get the labels from |mOptionRows| in the test, or if relying on the order of entries in there is too brittle for you, put just the payment option rows into a separate array. That way you also wouldn't need the payments_first_radio_button ID below.
Thanks! Could you please take another look? https://codereview.chromium.org/2120973002/diff/340001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/340001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:906: if (i < OPTION_LABEL_IDS.length) { On 2016/07/08 13:46:34, Bernhard Bauer wrote: > Having to reserve ID resources for these labels just for testing seems a bit > involved. You could get the labels from |mOptionRows| in the test, or if relying > on the order of entries in there is too brittle for you, put just the payment > option rows into a separate array. That way you also wouldn't need the > payments_first_radio_button ID below. Great idea, thanks. I did it for the suggestion labels that I just added. I created a bug to change the radio button too because it needs a bit more work.
Nice! Just some small suggestions: https://codereview.chromium.org/2120973002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:528: private final TextView[] mLabelsForTest = new TextView[5]; Make this a List<>, so you don't need to decide on a size in advance? https://codereview.chromium.org/2120973002/diff/360001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:214: protected String getContactDetailsSuggestionLabel(final int suggestionIndex) Maybe add a method here that returns the number of option labels, and then remove the null checks?
Patchset #11 (id:380001) has been deleted
Thanks, Another look? https://codereview.chromium.org/2120973002/diff/360001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/360001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:528: private final TextView[] mLabelsForTest = new TextView[5]; On 2016/07/12 08:38:49, Bernhard Bauer wrote: > Make this a List<>, so you don't need to decide on a size in advance? Done. https://codereview.chromium.org/2120973002/diff/360001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2120973002/diff/360001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:214: protected String getContactDetailsSuggestionLabel(final int suggestionIndex) On 2016/07/12 08:38:50, Bernhard Bauer wrote: > Maybe add a method here that returns the number of option labels, and then > remove the null checks? Done.
LGTM! https://codereview.chromium.org/2120973002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:928: ? null : mLabelsForTest.get(labelIndex); I would just remove the null here -- if the index was out of range, you would get an exception, which would register as a test failure.
Thanks, will send to CQ https://codereview.chromium.org/2120973002/diff/400001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java (right): https://codereview.chromium.org/2120973002/diff/400001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestSection.java:928: ? null : mLabelsForTest.get(labelIndex); On 2016/07/13 08:36:16, Bernhard Bauer wrote: > I would just remove the null here -- if the index was out of range, you would > get an exception, which would register as a test failure. Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org, bauerb@chromium.org Link to the patchset: https://codereview.chromium.org/2120973002/#ps420001 (title: "Removed the null return value")
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
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...)
The CQ bit was checked by sebsg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from
==========
Suggest complete addresses and contact details first and limit to the top four
suggestions.
BUG=625565
TEST=PaymentRequestDynamicShippingMultipleAddressesTest,
PaymentRequestMultipleContactDetailTest
==========
to
==========
Suggest complete addresses and contact details first and limit to the top four
suggestions.
BUG=625565
TEST=PaymentRequestDynamicShippingMultipleAddressesTest,
PaymentRequestMultipleContactDetailTest
==========
Message was sent while issue was closed.
Committed patchset #12 (id:420001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from
==========
Suggest complete addresses and contact details first and limit to the top four
suggestions.
BUG=625565
TEST=PaymentRequestDynamicShippingMultipleAddressesTest,
PaymentRequestMultipleContactDetailTest
==========
to
==========
Suggest complete addresses and contact details first and limit to the top four
suggestions.
BUG=625565
TEST=PaymentRequestDynamicShippingMultipleAddressesTest,
PaymentRequestMultipleContactDetailTest
Committed: https://crrev.com/40dd55f9afba8922e99f8e773ce6fbb980f42196
Cr-Commit-Position: refs/heads/master@{#405302}
==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/40dd55f9afba8922e99f8e773ce6fbb980f42196 Cr-Commit-Position: refs/heads/master@{#405302} |
