|
|
Created:
3 years, 8 months ago by sebsg Modified:
3 years, 8 months ago CC:
chromium-reviews, gogerald+paymentswatch_chromium.org, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, sebsg+paymentswatch_chromium.org, tfarina Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Normalize billing address for response on Desktop.
BUG=714646
Review-Url: https://codereview.chromium.org/2842463002
Cr-Commit-Position: refs/heads/master@{#467416}
Committed: https://chromium.googlesource.com/chromium/src/+/4540f48b0b6a2511b938b09986be4baee13a1520
Patch Set 1 #Patch Set 2 : Keep the old version for iOS #
Total comments: 8
Patch Set 3 : Added tests and addresses comments #Patch Set 4 : Rebase + Rebase fixes #Patch Set 5 : iOS fix #
Total comments: 8
Patch Set 6 : Addressed comments #
Total comments: 4
Patch Set 7 : Addressed moe's comments #Messages
Total messages: 62 (49 generated)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (id:1) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + anthonyvd@chromium.org
Hi Anthony, PTAL?
lgtm
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Math, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
can we test the new behavior in autofill_payment_instrument? https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... File components/payments/core/autofill_payment_instrument.cc (right): https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/autofill_payment_instrument.cc:56: // TODO(crbug.com/602666): Ensure we reach here only if the card has a billing TODO(crbug.com/709776): Make sure the billing address is valid before continuing. https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/autofill_payment_instrument.cc:62: DCHECK(billing_address); as discussed this morning let's avoid a crash and check for validity of |billing_address| before continuing. https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/autofill_payment_instrument.cc:123: if (is_waiting_for_billing_address_normalization_) { DCHECK? https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... File components/payments/core/payment_request_data_util.h (right): https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util.h:40: // TODO(crbug.com/714655): Once the billing address is checked in the instrument not sure I like this fork very much. can you move the code that gets the billing address to be used on iOS too? Or perhaps the ideal state is for AutofillPaymentInstrument to be used on iOS, but that's a bigger change.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Patchset #3 (id:100001) has been deleted
Patchset #3 (id:120001) has been deleted
Patchset #3 (id:140001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:160001) has been deleted
Thanks Math, another look? https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... File components/payments/core/autofill_payment_instrument.cc (right): https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/autofill_payment_instrument.cc:56: // TODO(crbug.com/602666): Ensure we reach here only if the card has a billing On 2017/04/24 19:42:09, Mathieu wrote: > TODO(crbug.com/709776): Make sure the billing address is valid before > continuing. Done. https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/autofill_payment_instrument.cc:62: DCHECK(billing_address); On 2017/04/24 19:42:09, Mathieu wrote: > as discussed this morning let's avoid a crash and check for validity of > |billing_address| before continuing. Done. https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/autofill_payment_instrument.cc:123: if (is_waiting_for_billing_address_normalization_) { On 2017/04/24 19:42:09, Mathieu wrote: > DCHECK? Done. https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... File components/payments/core/payment_request_data_util.h (right): https://codereview.chromium.org/2842463002/diff/60001/components/payments/cor... components/payments/core/payment_request_data_util.h:40: // TODO(crbug.com/714655): Once the billing address is checked in the instrument On 2017/04/24 19:42:09, Mathieu wrote: > not sure I like this fork very much. can you move the code that gets the billing > address to be used on iOS too? > > Or perhaps the ideal state is for AutofillPaymentInstrument to be used on iOS, > but that's a bigger change. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) 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 sebsg@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... File components/payments/core/autofill_payment_instrument.cc (right): https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... components/payments/core/autofill_payment_instrument.cc:111: if (!is_waiting_for_billing_address_normalization_) { no curlies https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... components/payments/core/autofill_payment_instrument.cc:128: if (!is_waiting_for_card_unmask_) { no curlies https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... File components/payments/core/autofill_payment_instrument_unittest.cc (right): https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... components/payments/core/autofill_payment_instrument_unittest.cc:48: bool on_instrument_details_ready_called_; can also use {false} or = false to initialize (not sure which or if both work) https://codereview.chromium.org/2842463002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2842463002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:224: autofill::AutofillProfile billing_address; Can you add a comment that if crbug.com/714768 is address there is no need to create a copy? We can simply pass *billing_address_ptr to the method.
sebsg@chromium.org changed reviewers: + eugenebut@chromium.org
eugenebut@chromium.org: Could you please take a look at payment_request_coordinator.mm ? Thanks! https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... File components/payments/core/autofill_payment_instrument.cc (right): https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... components/payments/core/autofill_payment_instrument.cc:111: if (!is_waiting_for_billing_address_normalization_) { On 2017/04/25 19:27:42, Mathieu wrote: > no curlies Done. https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... components/payments/core/autofill_payment_instrument.cc:128: if (!is_waiting_for_card_unmask_) { On 2017/04/25 19:27:42, Mathieu wrote: > no curlies Done. https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... File components/payments/core/autofill_payment_instrument_unittest.cc (right): https://codereview.chromium.org/2842463002/diff/220001/components/payments/co... components/payments/core/autofill_payment_instrument_unittest.cc:48: bool on_instrument_details_ready_called_; On 2017/04/25 19:27:42, Mathieu wrote: > can also use {false} or = false to initialize (not sure which or if both work) Done. https://codereview.chromium.org/2842463002/diff/220001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2842463002/diff/220001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:224: autofill::AutofillProfile billing_address; On 2017/04/25 19:27:42, Mathieu wrote: > Can you add a comment that if crbug.com/714768 is address there is no need to > create a copy? We can simply pass *billing_address_ptr to the method. Done.
Description was changed from ========== [Payments] Normalize billing address for response on Desktop. BUG=714646 ========== to ========== [Payments] Normalize billing address for response on Desktop. BUG=714646 ==========
sebsg@chromium.org changed reviewers: - eugenebut@chromium.org
sebsg@chromium.org changed reviewers: + lpromero@chromium.org
lpromero@chromium.org: Could you please take a look at payment_request_coordinator.mm? Thanks!
sebsg@chromium.org changed reviewers: - lpromero@chromium.org
sebsg@chromium.org changed reviewers: + mahmadi@chromium.org
On 2017/04/25 19:36:06, sebsg wrote: > mailto:lpromero@chromium.org: Could you please take a look at > payment_request_coordinator.mm? Thanks! lgtm % comments.
https://codereview.chromium.org/2842463002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2842463002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:224: autofill::AutofillProfile billing_address; s/billing_address/billingAddress https://codereview.chromium.org/2842463002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:232: autofill::PersonalDataManager::GetProfileFromProfilesByGUID( s/billing_address_ptr/billingAddressPtr
Thanks, sending to CQ https://codereview.chromium.org/2842463002/diff/240001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/payments/payment_request_coordinator.mm (right): https://codereview.chromium.org/2842463002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:224: autofill::AutofillProfile billing_address; On 2017/04/26 18:03:49, moe wrote: > s/billing_address/billingAddress Done. https://codereview.chromium.org/2842463002/diff/240001/ios/chrome/browser/ui/... ios/chrome/browser/ui/payments/payment_request_coordinator.mm:232: autofill::PersonalDataManager::GetProfileFromProfilesByGUID( On 2017/04/26 18:03:49, moe wrote: > s/billing_address_ptr/billingAddressPtr Done.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from anthonyvd@chromium.org, mathp@chromium.org, mahmadi@chromium.org Link to the patchset: https://codereview.chromium.org/2842463002/#ps260001 (title: "Addressed moe's comments")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 260001, "attempt_start_ts": 1493231310475980, "parent_rev": "b932d5ad0c349295c9d10de144f9358370e57b5c", "commit_rev": "4540f48b0b6a2511b938b09986be4baee13a1520"}
Message was sent while issue was closed.
Description was changed from ========== [Payments] Normalize billing address for response on Desktop. BUG=714646 ========== to ========== [Payments] Normalize billing address for response on Desktop. BUG=714646 Review-Url: https://codereview.chromium.org/2842463002 Cr-Commit-Position: refs/heads/master@{#467416} Committed: https://chromium.googlesource.com/chromium/src/+/4540f48b0b6a2511b938b09986be... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:260001) as https://chromium.googlesource.com/chromium/src/+/4540f48b0b6a2511b938b09986be... |