Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2590583002:
[Payment Request] Update the Contacts section after editing address (Closed)
|
Created:
4 years ago by Mathieu Modified:
3 years, 11 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payment Request] Update the Contacts section after editing address
Previous, a change to (or a new) shipping address would not update the
Contact Details section, resulting in the person having to re-enter their
details.
Now, when the address editor exits we add/replace the profile in the
collection and rebuild the Contact Details section.
BUG=675669
TEST=PaymentRequest*Shipping*
Review-Url: https://codereview.chromium.org/2590583002
Cr-Commit-Position: refs/heads/master@{#442311}
Committed: https://chromium.googlesource.com/chromium/src/+/7d8d2d7aab2598436338694558ac04c7de92b179
Patch Set 1 : Initial #
Total comments: 2
Patch Set 2 : Moved to ContactSection #Patch Set 3 : unit test #
Total comments: 26
Patch Set 4 : removed format diff #Patch Set 5 : moved code around #Patch Set 6 : rebase on top of Seb's change #
Total comments: 18
Patch Set 7 : Addressed comments #
Total comments: 2
Patch Set 8 : fix providePaymentInformation #Messages
Total messages: 39 (25 generated)
Description was changed from ========== [Payment Request] Update the Contacts section after editing address Previous, a change to (or a new) shipping address would not update the Contact Details section, resulting in the person having to re-enter their details. Now, when the address editor exits we add/replace the profile in the collection and rebuild the Contact Details section. x BUG=675669 TEST=PaymentRequest*Shipping* ========== to ========== [Payment Request] Update the Contacts section after editing address Previous, a change to (or a new) shipping address would not update the Contact Details section, resulting in the person having to re-enter their details. Now, when the address editor exits we add/replace the profile in the collection and rebuild the Contact Details section. BUG=675669 TEST=PaymentRequest*Shipping* ==========
mathp@chromium.org changed reviewers: + rouslan@chromium.org
Patchset #1 (id:1) has been deleted
Hi Rouslan, PTAL
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for working on this :-) https://codereview.chromium.org/2590583002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2590583002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:948: mCardEditor.updateBillingAddressIfComplete(editedAddress); Can you do mContactEditor.updateContact(editedAddress) instead? ;-) mCardEditor.updateBillingAddressIfComplete(editedAddress) either updates an existing billing address or adds a new billing address to its list.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:40001) has been deleted
Thanks Rouslan, let me know what you think of this approach! https://codereview.chromium.org/2590583002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2590583002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:948: mCardEditor.updateBillingAddressIfComplete(editedAddress); On 2016/12/19 20:40:07, rouslan-intermittent-holidays wrote: > Can you do mContactEditor.updateContact(editedAddress) instead? ;-) > > mCardEditor.updateBillingAddressIfComplete(editedAddress) either updates an > existing billing address or adds a new billing address to its list. Moved some logic to the new ContactSection.
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Rouslan, added a unit test, PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Meta-comment: let's not resort contact section when a new contact info is added or an old one is updated. This messes up the existing selection. See details inline. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:256: private ContactDetailsSection mContactDetailsSection; Your match will be smaller if you keep the name mContectSection. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:293: mOrigin = Please don't reformat lines that did not change. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:359: profiles = PersonalDataManager.getInstance().getProfilesToSuggest( profiles = Collections.unmodifiableList(PersonaDataManager...) (This catches a lot of bugs.) https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:915: // will reflow comment please https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:927: providePaymentInformation(); What problem does moving this statement out of "else" solve? https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java (right): https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:60: public void updateWithAutofillAddress(final AutofillAddress editedAddress) { Does not appear to be a need for finality here. The final keyword makes sure that you cannot write "editedAddress = someOtherAddress". It does not provide protection against something like "editedAddress.setField(someValue)". https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:67: mProfiles.add(0, editedAddress.getProfile()); Need to update selected item index in the parent. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:75: mProfiles.add(0, editedAddress.getProfile()); Ditto https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:79: updateContactListFromAutofillProfiles(); Let's not cut-off the bottom contact information after adding a new contact. Consider this situation: 1) User selects the last contact info 2) User adds a new profile in address editor. Expected: The last contact info is still present and selected. Observed: The last contact info is gone. A contact is no longer selected. In the same vein, perhaps we should not re-sort contacts after creation. That would make the whole index business easier. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:83: private void updateContactListFromAutofillProfiles() { Need to update selected item index after the sorting if this is called due to address update. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:108: String uniqueContactInfo = name + phone + email; FYI, Seb is working on improving this algorithm. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:132: // Automatically select the first address if it is complete. s/address/contact/
Patchset #5 (id:120001) has been deleted
Thanks, PTAL https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:256: private ContactDetailsSection mContactDetailsSection; On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > Your match will be smaller if you keep the name mContectSection. Done. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:293: mOrigin = On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > Please don't reformat lines that did not change. Done. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:359: profiles = PersonalDataManager.getInstance().getProfilesToSuggest( On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > profiles = Collections.unmodifiableList(PersonaDataManager...) > > (This catches a lot of bugs.) Done. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:915: // will On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > reflow comment please Done. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:927: providePaymentInformation(); On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > What problem does moving this statement out of "else" solve? The "Checking..." spinner would endlessly show if providePaymentInformation() was not called in the 957-964 block. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java (right): https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:60: public void updateWithAutofillAddress(final AutofillAddress editedAddress) { On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > Does not appear to be a need for finality here. The final keyword makes sure > that you cannot write "editedAddress = someOtherAddress". It does not provide > protection against something like "editedAddress.setField(someValue)". Done. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:67: mProfiles.add(0, editedAddress.getProfile()); On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > Need to update selected item index in the parent. Acknowledged. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:75: mProfiles.add(0, editedAddress.getProfile()); On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > Ditto Acknowledged. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:79: updateContactListFromAutofillProfiles(); On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > Let's not cut-off the bottom contact information after adding a new contact. > Consider this situation: > > 1) User selects the last contact info > 2) User adds a new profile in address editor. > > Expected: The last contact info is still present and selected. > > Observed: The last contact info is gone. A contact is no longer selected. > > In the same vein, perhaps we should not re-sort contacts after creation. That > would make the whole index business easier. Acknowledged. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:83: private void updateContactListFromAutofillProfiles() { On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > Need to update selected item index after the sorting if this is called due to > address update. Acknowledged. https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:108: String uniqueContactInfo = name + phone + email; On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > FYI, Seb is working on improving this algorithm. Yep. Hopefully he can just drop his code in here and update the tests https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:132: // Automatically select the first address if it is complete. On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > s/address/contact/ Done.
https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2590583002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:927: providePaymentInformation(); On 2016/12/20 20:21:41, Mathieu (OOO Dec 21-Jan 4) wrote: > On 2016/12/20 18:45:21, rouslan-intermittent-holidays wrote: > > What problem does moving this statement out of "else" solve? > > The "Checking..." spinner would endlessly show if providePaymentInformation() > was not called in the 957-964 block. The "Checking..." spinner is removed in updateWith() call on line 549. The spinner is necessary to block user interaction with UI while the website verifies the new shipping address. If your "Checking..." spinner is spinning forever, then your JavaScript did not register for shipping address change events or did not respond to those events in 60 seconds. Please fix your test instead JavaScript code instead. https://codereview.chromium.org/2590583002/diff/80001/chrome/test/data/androi... File chrome/test/data/android/payments/contact_details_and_free_shipping.js (right): https://codereview.chromium.org/2590583002/diff/80001/chrome/test/data/androi... chrome/test/data/android/payments/contact_details_and_free_shipping.js:27: requestPayerPhone: true, requestShipping: true}); If the shipping address is going to change in your test, JavaScript should listen for 'shippingAddressChange' event and respond to it with updated details. Otherwise your UI will time out.
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Hi Rouslan, I rebased on top of Seb's change. We should be good to do, PTAL
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:109: public boolean isEqualOrSupersetOf(final AutofillContact contact) { The "final" keyword here is not necessary. It prevents reassigning the |contact| pointer to a different instance of AutofillContact, but does not provide any other benefits. For example, the "final" keyword does not prevent modification of the data inside of this instance of AutofillContact. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:166: public static final int SUGGESTIONS_LIMIT = 4; nit: newline after this constant, so that its comment does not appear to be a section comment. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:927: providePaymentInformation(); Putting providePaymentInformation() here will allow the user to interact with the UI (and even click "Pay") before the website responds to the updated shipping address, for example with new shipping price. Instead, you can fix the test timeout by listening to 'shippingAddressChange' event in the test JavaScript file. See https://rsolomakhin.github.io/pr/single/pr.js for example. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1018: mContactSection != null ? mContactSection.getSelectedItem() : null; nit: undo reformat. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:39: * @param autofillProfiles The list of profiles to build from. JavaDoc @param name should match the code. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:78: break; You can return instead of break. This also removes the need for the contactExisted boolean. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:215: mItems.addAll(itemCollection); Does this work? mItems = new ArrayList<>(itemCollection); https://codereview.chromium.org/2590583002/diff/160001/chrome/test/data/andro... File chrome/test/data/android/payments/contact_details_and_free_shipping.js (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/test/data/andro... chrome/test/data/android/payments/contact_details_and_free_shipping.js:27: requestPayerPhone: true, requestShipping: true}); Add these lines to avoid timeouts when address changes: request.addEventListener('shippingaddresschange', function(e) { e.updateWith(new Promise(function(resolve) { // No changes in price based on shipping address change. resolve(details); })); }); The |details| variable is the second parameter to the PaymentRequest() constructor. See https://rsolomakhin.github.io/pr/single/pr.js for a complete example. https://codereview.chromium.org/2590583002/diff/160001/chrome/test/data/andro... chrome/test/data/android/payments/contact_details_and_free_shipping.js:32: print( FYI, these 7 lines can now be replaced with: print(JSON.stringify(resp, undefined, 2));
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mathp@chromium.org to run a CQ dry run
Thanks, addressed the comments https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillContact.java:109: public boolean isEqualOrSupersetOf(final AutofillContact contact) { On 2017/01/09 15:35:55, rouslan wrote: > The "final" keyword here is not necessary. It prevents reassigning the |contact| > pointer to a different instance of AutofillContact, but does not provide any > other benefits. For example, the "final" keyword does not prevent modification > of the data inside of this instance of AutofillContact. Done. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:166: public static final int SUGGESTIONS_LIMIT = 4; On 2017/01/09 15:35:55, rouslan wrote: > nit: newline after this constant, so that its comment does not appear to be a > section comment. Done. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:927: providePaymentInformation(); On 2017/01/09 15:35:55, rouslan wrote: > Putting providePaymentInformation() here will allow the user to interact with > the UI (and even click "Pay") before the website responds to the updated > shipping address, for example with new shipping price. Instead, you can fix the > test timeout by listening to 'shippingAddressChange' event in the test > JavaScript file. See https://rsolomakhin.github.io/pr/single/pr.js for example. Oh that's a good catch, thanks! https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1018: mContactSection != null ? mContactSection.getSelectedItem() : null; On 2017/01/09 15:35:55, rouslan wrote: > nit: undo reformat. Done. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:39: * @param autofillProfiles The list of profiles to build from. On 2017/01/09 15:35:55, rouslan wrote: > JavaDoc @param name should match the code. Done. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/ContactDetailsSection.java:78: break; On 2017/01/09 15:35:55, rouslan wrote: > You can return instead of break. This also removes the need for the > contactExisted boolean. Done. https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/SectionInformation.java:215: mItems.addAll(itemCollection); On 2017/01/09 15:35:55, rouslan wrote: > Does this work? > > mItems = new ArrayList<>(itemCollection); Done. https://codereview.chromium.org/2590583002/diff/160001/chrome/test/data/andro... File chrome/test/data/android/payments/contact_details_and_free_shipping.js (right): https://codereview.chromium.org/2590583002/diff/160001/chrome/test/data/andro... chrome/test/data/android/payments/contact_details_and_free_shipping.js:27: requestPayerPhone: true, requestShipping: true}); On 2017/01/09 15:35:55, rouslan wrote: > Add these lines to avoid timeouts when address changes: > > request.addEventListener('shippingaddresschange', function(e) { > e.updateWith(new Promise(function(resolve) { > // No changes in price based on shipping address change. > resolve(details); > })); > }); > > The |details| variable is the second parameter to the PaymentRequest() > constructor. See https://rsolomakhin.github.io/pr/single/pr.js for a complete > example. Done. https://codereview.chromium.org/2590583002/diff/160001/chrome/test/data/andro... chrome/test/data/android/payments/contact_details_and_free_shipping.js:32: print( On 2017/01/09 15:35:55, rouslan wrote: > FYI, these 7 lines can now be replaced with: > > print(JSON.stringify(resp, undefined, 2)); Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM % comment https://codereview.chromium.org/2590583002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (left): https://codereview.chromium.org/2590583002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:992: providePaymentInformation(); Let's keep the "} else { providePaymentInformation(); }" to handle the case when the address editor returns null address. That happens when user clicked "Add address" and then "Cancel". I was warning only against an unconditional call to providePaymentInformation().
Thanks, submitting. https://codereview.chromium.org/2590583002/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (left): https://codereview.chromium.org/2590583002/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:992: providePaymentInformation(); On 2017/01/09 17:58:22, rouslan wrote: > Let's keep the "} else { providePaymentInformation(); }" to handle the case when > the address editor returns null address. That happens when user clicked "Add > address" and then "Cancel". I was warning only against an unconditional call to > providePaymentInformation(). Sorry I got distracted. Thanks for catching this :\
The CQ bit was checked by mathp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rouslan@chromium.org Link to the patchset: https://codereview.chromium.org/2590583002/#ps200001 (title: "fix providePaymentInformation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 200001, "attempt_start_ts": 1483985109994110,
"parent_rev": "192db0bda7b218571745eea5ca72197925142524", "commit_rev":
"7d8d2d7aab2598436338694558ac04c7de92b179"}
Message was sent while issue was closed.
Description was changed from ========== [Payment Request] Update the Contacts section after editing address Previous, a change to (or a new) shipping address would not update the Contact Details section, resulting in the person having to re-enter their details. Now, when the address editor exits we add/replace the profile in the collection and rebuild the Contact Details section. BUG=675669 TEST=PaymentRequest*Shipping* ========== to ========== [Payment Request] Update the Contacts section after editing address Previous, a change to (or a new) shipping address would not update the Contact Details section, resulting in the person having to re-enter their details. Now, when the address editor exits we add/replace the profile in the collection and rebuild the Contact Details section. BUG=675669 TEST=PaymentRequest*Shipping* Review-Url: https://codereview.chromium.org/2590583002 Cr-Commit-Position: refs/heads/master@{#442311} Committed: https://chromium.googlesource.com/chromium/src/+/7d8d2d7aab2598436338694558ac... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:200001) as https://chromium.googlesource.com/chromium/src/+/7d8d2d7aab2598436338694558ac... |
