Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2451303002:
[Payments] Don't add expired cards in Payment Request. (Closed)
Description[Payments] Don't add expired cards in Payment Request.
BUG=658823
Committed: https://crrev.com/9517dee1565a1629116b9e30c43c75ba65d510cb
Cr-Commit-Position: refs/heads/master@{#428164}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Addressed comments #
Total comments: 10
Patch Set 3 : Addressed comments #
Depends on Patchset: Messages
Total messages: 23 (12 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, ptal?
https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:410: public boolean isValid(@Nullable CharSequence value) { Please rename |value| to |unusedValue| so it's clear that we're not using it. Then add a comment that this validator runs after changes in either month or year dropdown, which is the reason for accessing those fields directly instead of using the parameter. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:416: Calendar now = Calendar.getInstance(); This can read from disk on UI thread, which will make the UI jumpy. See lines 278-290 for the correct way to access a Calendar instance. You may be able to put that in a shared helper function. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:440: if (mObserverForTest != null) { Surround lines 437-444 with "if (mObserverForTest != null)" check instead. That way production code does not have to create a Callback object that does nothing. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:101: * the editor UI. "but does not alter the editor UI" on lines 94-95 refers to CardEditor launching an address editor when user selects "+ add billing address" in the dropdown. Nothing crazy like this happens for expiration month dropdown, so there's no need to have that phrasing. You can probably simplify it to "Called when expiration month changed in credit card editor." You can also remove the "Processed" part of the method name, because nothing fancy happens when the dropdown value is changed. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:199: * Constructs a dropdown field model. ... with a validator. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:204: * @param validator Optional validator for the values in this field. Let's make it non-optional. If the user of this API does not want to have a validator, they should be using the validator-less function above. Also remove @Nullable for validator and invalidErrorMessage and add an "assert xyz != null" in the body of the function. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:207: */ Comment indent. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2579: <message name="IDS_PAYMENTS_CARD_EXPIRATION_INVALID_VALIDATION_MESSAGE" desc="The text that informs the user that the credit card expiration date they hav Spurious newline? https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestExpiredLocalCardTest.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestExpiredLocalCardTest.java:95: Calendar now = Calendar.getInstance(); This will read from disk, but this time on instrumentation thread. You shooooould be OK, but no guarantees. If an Android bot goes red because of a StrictMode violation in this test, you fix it :-P https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:82: protected final CallbackHelper mExpirationMonthChangeProcessed; mExpirationMonthChanged
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 #2 (id:60001) has been deleted
Thanks! another look? https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:410: public boolean isValid(@Nullable CharSequence value) { On 2016/10/27 00:05:22, rouslan wrote: > Please rename |value| to |unusedValue| so it's clear that we're not using it. > Then add a comment that this validator runs after changes in either month or > year dropdown, which is the reason for accessing those fields directly instead > of using the parameter. Realized that now this validator is only used for credit card month, so I can actually use the value. Thanks! https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:416: Calendar now = Calendar.getInstance(); On 2016/10/27 00:05:22, rouslan wrote: > This can read from disk on UI thread, which will make the UI jumpy. See lines > 278-290 for the correct way to access a Calendar instance. You may be able to > put that in a shared helper function. Done. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:440: if (mObserverForTest != null) { On 2016/10/27 00:05:22, rouslan wrote: > Surround lines 437-444 with "if (mObserverForTest != null)" check instead. That > way production code does not have to create a Callback object that does nothing. Done. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:101: * the editor UI. On 2016/10/27 00:05:22, rouslan wrote: > "but does not alter the editor UI" on lines 94-95 refers to CardEditor launching > an address editor when user selects "+ add billing address" in the dropdown. > Nothing crazy like this happens for expiration month dropdown, so there's no > need to have that phrasing. You can probably simplify it to "Called when > expiration month changed in credit card editor." You can also remove the > "Processed" part of the method name, because nothing fancy happens when the > dropdown value is changed. Done. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:199: * Constructs a dropdown field model. On 2016/10/27 00:05:22, rouslan wrote: > ... with a validator. Done. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:204: * @param validator Optional validator for the values in this field. On 2016/10/27 00:05:22, rouslan wrote: > Let's make it non-optional. If the user of this API does not want to have a > validator, they should be using the validator-less function above. Also remove > @Nullable for validator and invalidErrorMessage and add an "assert xyz != null" > in the body of the function. Done. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/EditorFieldModel.java:207: */ On 2016/10/27 00:05:22, rouslan wrote: > Comment indent. Done. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/str... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/java/str... chrome/android/java/strings/android_chrome_strings.grd:2579: <message name="IDS_PAYMENTS_CARD_EXPIRATION_INVALID_VALIDATION_MESSAGE" desc="The text that informs the user that the credit card expiration date they hav On 2016/10/27 00:05:22, rouslan wrote: > Spurious newline? Done. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestExpiredLocalCardTest.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestExpiredLocalCardTest.java:95: Calendar now = Calendar.getInstance(); On 2016/10/27 00:05:23, rouslan wrote: > This will read from disk, but this time on instrumentation thread. You > shooooould be OK, but no guarantees. If an Android bot goes red because of a > StrictMode violation in this test, you fix it :-P Acknowledged. https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/2451303002/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:82: protected final CallbackHelper mExpirationMonthChangeProcessed; On 2016/10/27 00:05:23, rouslan wrote: > mExpirationMonthChanged Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:411: No need for the extra newline https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:417: EditorFieldValidator cardExpirationMonthValidator = new EditorFieldValidator() { Let's keep this in a lazy-initialized member variable, so multiple instantiations of card editor re-use the same object instead of creating a new one every time. Object creation is expensive. https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:423: } else { You can save a line or two by using early return instead of if-return-else-do-something-else. if (montValue == null || yearValue == null) return false; int month = Integer.parseInt... https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:438: R.string.payments_card_expiration_invalid_validation_message)); "invalid_validation_message" sounds funky. What about just "invalid"? R.string.payments_card_expiration_invalid https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:100: * Called when the controller is notified of an expiration month change. */ Put */ on a new line?
Addressed, thanks! https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java (right): https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:411: On 2016/10/27 15:34:05, rouslan wrote: > No need for the extra newline Done. https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:417: EditorFieldValidator cardExpirationMonthValidator = new EditorFieldValidator() { On 2016/10/27 15:34:05, rouslan wrote: > Let's keep this in a lazy-initialized member variable, so multiple > instantiations of card editor re-use the same object instead of creating a new > one every time. Object creation is expensive. Done. https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:423: } else { On 2016/10/27 15:34:05, rouslan wrote: > You can save a line or two by using early return instead of > if-return-else-do-something-else. > > if (montValue == null || yearValue == null) return false; > int month = Integer.parseInt... Done. https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/CardEditor.java:438: R.string.payments_card_expiration_invalid_validation_message)); On 2016/10/27 15:34:05, rouslan wrote: > "invalid_validation_message" sounds funky. What about just "invalid"? > > R.string.payments_card_expiration_invalid I followed the convention used in other messages of this type. We could maybe change them all at the same time in another patch? https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://codereview.chromium.org/2451303002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:100: * Called when the controller is notified of an expiration month change. */ On 2016/10/27 15:34:05, rouslan wrote: > Put */ on a new line? Done.
Patchset #3 (id:100001) has been deleted
lgtm
sebsg@chromium.org changed reviewers: + dfalcantara@chromium.org
Hi Dan, could you please review android_chrome_strings.grd ? Thanks!
Strings lgtm
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.
Committed patchset #3 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== [Payments] Don't add expired cards in Payment Request. BUG=658823 ========== to ========== [Payments] Don't add expired cards in Payment Request. BUG=658823 Committed: https://crrev.com/9517dee1565a1629116b9e30c43c75ba65d510cb Cr-Commit-Position: refs/heads/master@{#428164} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9517dee1565a1629116b9e30c43c75ba65d510cb Cr-Commit-Position: refs/heads/master@{#428164} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
