Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2659763002:
[Payments] Allow cancel payment request when loading instruments (Closed)
|
Created:
3 years, 10 months ago by gogerald1 Modified:
3 years, 10 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. |
DescriptionAllow cancel payment request when loading instruments
This CL also removed an unnecessary boolean.
BUG=685771
Review-Url: https://codereview.chromium.org/2659763002
Cr-Commit-Position: refs/heads/master@{#446988}
Committed: https://chromium.googlesource.com/chromium/src/+/9715fde14205696ecccf255cbf342f551c6b9cce
Patch Set 1 #
Total comments: 6
Patch Set 2 : address comments #
Messages
Total messages: 27 (19 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Wait for pay processing BUG= ========== to ========== Clean up for code health mButtonBar is set to gone when showing spinner, so set of mCloseButton is not needed. mIsProcessingPayClicked should be true when showing processing message since the payment has not been completely processed yet. BUG=685771 ==========
Description was changed from ========== Clean up for code health mButtonBar is set to gone when showing spinner, so set of mCloseButton is not needed. mIsProcessingPayClicked should be true when showing processing message since the payment has not been completely processed yet. BUG=685771 ========== to ========== Clean up for code health mButtonBar is set to gone when showing spinner, so set of mCloseButton is not needed. mIsProcessingPayClicked should be true when showing processing message since the payment has not been completely processed yet. BUG=685771 ==========
The CQ bit was checked by gogerald@chromium.org to run a CQ dry run
gogerald@chromium.org changed reviewers: + rouslan@chromium.org
Hi rouslan@, PTAL,
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 you record the videos of the "before" and "after" behavior? I wonder what you're trying to fix with the change of mIsPorcessingPayClicked. (I was not aware that anything was broken there.)
On 2017/01/27 16:16:49, rouslan wrote: > Can you record the videos of the "before" and "after" behavior? I wonder what > you're trying to fix with the change of mIsPorcessingPayClicked. (I was not > aware that anything was broken there.) Nothing is broken as mentioned offline. Just want to make the code do right and necessary things. The behavior from the user point of view is the same before and after this CL.
https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1057: if (mPaymentMethodsSection.getSelectedItem() instanceof AutofillPaymentInstrument) { You should make this an assert instead, because this onInstrumentDetailsLoadingWithoutUI() is called only from AutofillPaymentInstrument: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (left): https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:908: mIsProcessingPayClicked = false; This looks like a harmless change. https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:912: mCloseButton.setVisibility(View.INVISIBLE); mCloseButton is not inside of the mButtonBar, so this code block should remain. (mClosebutton is on the top-right. mButtonBar is on the bottom of the UI.)
Description was changed from ========== Clean up for code health mButtonBar is set to gone when showing spinner, so set of mCloseButton is not needed. mIsProcessingPayClicked should be true when showing processing message since the payment has not been completely processed yet. BUG=685771 ========== to ========== Clean up for code health Move control of mCloseButton to changeSpinnerVisibility. mIsProcessingPayClicked should be true when showing processing message since the payment has not been completely processed yet. BUG=685771 ==========
Patchset #2 (id:40001) has been deleted
Patchset #2 (id:60001) has been deleted
Description was changed from ========== Clean up for code health Move control of mCloseButton to changeSpinnerVisibility. mIsProcessingPayClicked should be true when showing processing message since the payment has not been completely processed yet. BUG=685771 ========== to ========== Allow cancel payment request when loading instruments This CL also removed an unnecessary boolean. BUG=685771 ==========
PTAL, https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:1057: if (mPaymentMethodsSection.getSelectedItem() instanceof AutofillPaymentInstrument) { On 2017/01/27 16:47:43, rouslan wrote: > You should make this an assert instead, because this > onInstrumentDetailsLoadingWithoutUI() is called only from > AutofillPaymentInstrument: > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Done. https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (left): https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:908: mIsProcessingPayClicked = false; On 2017/01/27 16:47:43, rouslan wrote: > This looks like a harmless change. Yes, but looks not as intended, The only use of mIsProcessingPayClicked is in isAcceptingCloseButton, if we set it false here, then isAcceptingCloseButton return true. However, we hided the close button immediately later. https://codereview.chromium.org/2659763002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:912: mCloseButton.setVisibility(View.INVISIBLE); On 2017/01/27 16:47:43, rouslan wrote: > mCloseButton is not inside of the mButtonBar, so this code block should remain. > (mClosebutton is on the top-right. mButtonBar is on the bottom of the UI.) Moved control of mCloseButton to changeSpinnerVisibility since it is shown with spinner only when loading instruments. After that the mCloseButton is hidden when spinner is shown.
The CQ bit was checked by rouslan@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.
lgtm
The CQ bit was checked by gogerald@chromium.org
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": 80001, "attempt_start_ts": 1485789411628220,
"parent_rev": "90a0232e3b790a6b80c6411bbdaeea3338df5e6c", "commit_rev":
"9715fde14205696ecccf255cbf342f551c6b9cce"}
Message was sent while issue was closed.
Description was changed from ========== Allow cancel payment request when loading instruments This CL also removed an unnecessary boolean. BUG=685771 ========== to ========== Allow cancel payment request when loading instruments This CL also removed an unnecessary boolean. BUG=685771 Review-Url: https://codereview.chromium.org/2659763002 Cr-Commit-Position: refs/heads/master@{#446988} Committed: https://chromium.googlesource.com/chromium/src/+/9715fde14205696ecccf255cbf34... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9715fde14205696ecccf255cbf34... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
