Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2925403002:
[Payments] Create and add AutofillPaymentApp before show is called. (Closed)
|
Created:
3 years, 6 months ago by gogerald1 Modified:
3 years, 6 months ago Reviewers:
please use gerrit instead CC:
chromium-reviews, mahmadi+paymentswatch_chromium.org, rouslan+payments_chromium.org, agrieve+watch_chromium.org, sebsg+paymentswatch_chromium.org, gogerald+paymentswatch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate and add AutofillPaymentApp deterministicaly before show is called.
BUG=730888
Review-Url: https://codereview.chromium.org/2925403002
Cr-Commit-Position: refs/heads/master@{#478309}
Committed: https://chromium.googlesource.com/chromium/src/+/5c01f655028db7670a72fdfefcb99282f08f56a3
Patch Set 1 #
Total comments: 2
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : move place #
Messages
Total messages: 21 (14 generated)
The CQ bit was checked by gogerald@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...
Description was changed from ========== Create and add AutofillPaymentApp early to figure out whether the merchant supports autofill payment instrument before show is called BUG= ========== to ========== Create and add AutofillPaymentApp deterministicaly before show is called. BUG=730888 ==========
gogerald@chromium.org changed reviewers: + rouslan@chromium.org
Hi rouslan@, ptal,
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for working on this! https://codereview.chromium.org/2925403002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2925403002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:482: mApps.add(autofillPaymentApp); Please create a static method AutofillPaymentApp.merchantSupportsAutofillPaymentInstruments(methodDataMap) and call that directly when creating PaymentRequestUI. That way we can remove mMerchantSupportsAutofillPaymentInstruments variable entirely. The supportsMethodsAndData(methodDataMap) method is defined in PaymentApp interface, so the @Override in AutofillPaymentApp must be dynamic. Please make AutofillPaymentApp.supportsMethodsAndData(methodDataMap) method call into the static method AutofillPaymentApp.merchantSupportsAutofillPaymentInstruments(methodDataMap).
The CQ bit was checked by gogerald@chromium.org
The CQ bit was unchecked by gogerald@chromium.org
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
another look? https://codereview.chromium.org/2925403002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2925403002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:482: mApps.add(autofillPaymentApp); On 2017/06/09 14:39:54, rouslan wrote: > Please create a static method > AutofillPaymentApp.merchantSupportsAutofillPaymentInstruments(methodDataMap) and > call that directly when creating PaymentRequestUI. That way we can remove > mMerchantSupportsAutofillPaymentInstruments variable entirely. > > The supportsMethodsAndData(methodDataMap) method is defined in PaymentApp > interface, so the @Override in AutofillPaymentApp must be dynamic. Please make > AutofillPaymentApp.supportsMethodsAndData(methodDataMap) method call into the > static method > AutofillPaymentApp.merchantSupportsAutofillPaymentInstruments(methodDataMap). good idea, but did not remove mMerchantSupportsAutofillPaymentInstruments since it is used to check whether we should disconnect from the client as well (might not good to check it again). In addition, we might want to merge this to M60
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM % nits https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java (right): https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:123: } The diff will be easier to compare of you move this static function immediately after the supportsMethodAndData() method below. https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:719: // Query instruments after mMerchantSupportsAutofillPaymentInstruments has been initialized, Please remove this comment.
Thanks, sending to CQ, https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java (right): https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/AutofillPaymentApp.java:123: } On 2017/06/09 15:49:50, rouslan wrote: > The diff will be easier to compare of you move this static function immediately > after the supportsMethodAndData() method below. Done. https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2925403002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:719: // Query instruments after mMerchantSupportsAutofillPaymentInstruments has been initialized, On 2017/06/09 15:49:51, rouslan wrote: > Please remove this comment. Done.
The CQ bit was checked by gogerald@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/2925403002/#ps40001 (title: "move place")
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": 40001, "attempt_start_ts": 1497023974378590,
"parent_rev": "9785570224c6e7ec857bb5084a887ea91350d95c", "commit_rev":
"5c01f655028db7670a72fdfefcb99282f08f56a3"}
Message was sent while issue was closed.
Description was changed from ========== Create and add AutofillPaymentApp deterministicaly before show is called. BUG=730888 ========== to ========== Create and add AutofillPaymentApp deterministicaly before show is called. BUG=730888 Review-Url: https://codereview.chromium.org/2925403002 Cr-Commit-Position: refs/heads/master@{#478309} Committed: https://chromium.googlesource.com/chromium/src/+/5c01f655028db7670a72fdfefcb9... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5c01f655028db7670a72fdfefcb9... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
