Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2165163002:
Autofill cards at the bottom of the PaymentRequest UI (Closed)
|
Created:
4 years, 5 months ago by please use gerrit instead Modified:
4 years, 5 months ago Reviewers:
gone CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutofill cards at the bottom of the PaymentRequest UI
This patch arranges the payment instruments in the following order:
> Non-autofill payment apps
> Complete credit cards from Autofill.
> Incomplete credit card from Autofill (need editing before sending to
the merchant).
BUG=629882
Committed: https://crrev.com/84204fda8113ffef6766812db30d6e11fee1269c
Cr-Commit-Position: refs/heads/master@{#407179}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Comments #Patch Set 3 : Static comparator and faux-parameterized test #Messages
Total messages: 25 (16 generated)
Description was changed from ========== Autofill card should be on the bottom of the PaymentRequest UI BUG=629882 ========== to ========== Autofill cards at the bottom of the PaymentRequest UI BUG=629882 ==========
Description was changed from ========== Autofill cards at the bottom of the PaymentRequest UI BUG=629882 ========== to ========== Autofill cards at the bottom of the PaymentRequest UI This patch arranges the payment instruments in the following order: > Non-autofill payment apps > Complete credit cards from Autofill. > Incomplete credit card from Autofill (need editing before sending to the merchant). BUG=629882 ==========
The CQ bit was checked by rouslan@chromium.org to run a CQ dry run
rouslan@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@, 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: 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_...)
https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:101: private Comparator<Completable> mCompletenessComparator; private final? https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:874: if (instruments != null) { Is this where the "Non-autofill instruments" are directly added to the list? Can you add a comment to that effect here, then add a function javadoc to addPendingInstrument() about it? https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java:92: installPaymentApp(HAVE_INSTRUMENTS, IMMEDIATE_RESPONSE); are these tests all basically the same? Does it make sense to just have a parameterized runTest function that takes (NO_INSTRUMENTS/HAVE_INSTRUMENTS, IMMEDIATE_RESPONSE/DELAYED_RESPONSE, numberOfPaymentInstruments)?
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...
dfalcantara@, ptal patch 2 and see my comments inline. https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:101: private Comparator<Completable> mCompletenessComparator; On 2016/07/21 21:24:42, dfalcantara wrote: > private final? Cannot be final because it may not be initialized if any of these are true: (1) webContents == null (2) ContentViewCore.fromWebContents(webContents) == null (3) ContentViewCore.fromWebContents(webContents).getWindowAndroid() == null (4) ContentViewCore.fromWebContents(webContents).getWindowAndroid().getActivity() == null Alternatively I can make the variable final if I initialize it inline where it's declared. However, I think that makes the code less readable, because initialization is 5 lines. https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:874: if (instruments != null) { On 2016/07/21 21:24:42, dfalcantara wrote: > Is this where the "Non-autofill instruments" are directly added to the list? > Can you add a comment to that effect here, then add a function javadoc to > addPendingInstrument() about it? Done. https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java:92: installPaymentApp(HAVE_INSTRUMENTS, IMMEDIATE_RESPONSE); On 2016/07/21 21:24:42, dfalcantara wrote: > are these tests all basically the same? Does it make sense to just have a > parameterized runTest function that takes (NO_INSTRUMENTS/HAVE_INSTRUMENTS, > IMMEDIATE_RESPONSE/DELAYED_RESPONSE, numberOfPaymentInstruments)? I can't use the junit parameterization classes in our integration tests. Do we have custom classes for this or it's flat out not supported? I assume we don't want to do this manually. PaymentRequestPaymentAppAndCardsTest.java:15: error: package org.junit.runner does not exist import org.junit.runner.RunWith; ^ PaymentRequestPaymentAppAndCardsTest.java:16: error: package org.junit.runners does not exist import org.junit.runners.Parameterized; ^ PaymentRequestPaymentAppAndCardsTest.java:17: error: package org.junit.runners.Parameterized does not exist import org.junit.runners.Parameterized.Parameters; ^ PaymentRequestPaymentAppAndCardsTest.java:27: error: cannot find symbol @RunWith(Parameterized.class) ^ symbol: class RunWith PaymentRequestPaymentAppAndCardsTest.java:29: error: cannot find symbol @Parameters ^ symbol: class Parameters location: class PaymentRequestPaymentAppAndCardsTest 5 errors
https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:101: private Comparator<Completable> mCompletenessComparator; On 2016/07/21 22:06:23, Rouslan (ツ) wrote: > On 2016/07/21 21:24:42, dfalcantara wrote: > > private final? > > Cannot be final because it may not be initialized if any of these are true: > > (1) webContents == null > > (2) ContentViewCore.fromWebContents(webContents) == null > > (3) ContentViewCore.fromWebContents(webContents).getWindowAndroid() == null > > (4) > ContentViewCore.fromWebContents(webContents).getWindowAndroid().getActivity() == > null > > > Alternatively I can make the variable final if I initialize it inline where it's > declared. However, I think that makes the code less readable, because > initialization is 5 lines. Yeah, I figured initializing it where it's declared would make the most sense, especially because it's not reliant on any of the other things. You could even make it a private static final Comparator<Completable> COMPLETEDNESS_COMPARATOR = new blah() { } so that it's shared among all the instances because it never changes. Will leave it to your judgement, though. https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java:92: installPaymentApp(HAVE_INSTRUMENTS, IMMEDIATE_RESPONSE); On 2016/07/21 22:06:23, Rouslan (ツ) wrote: > On 2016/07/21 21:24:42, dfalcantara wrote: > > are these tests all basically the same? Does it make sense to just have a > > parameterized runTest function that takes (NO_INSTRUMENTS/HAVE_INSTRUMENTS, > > IMMEDIATE_RESPONSE/DELAYED_RESPONSE, numberOfPaymentInstruments)? > > I can't use the junit parameterization classes in our integration tests. Do we > have custom classes for this or it's flat out not supported? I assume we don't > want to do this manually. > > PaymentRequestPaymentAppAndCardsTest.java:15: error: package org.junit.runner > does not exist > import org.junit.runner.RunWith; > ^ > PaymentRequestPaymentAppAndCardsTest.java:16: error: package org.junit.runners > does not exist > import org.junit.runners.Parameterized; > ^ > PaymentRequestPaymentAppAndCardsTest.java:17: error: package > org.junit.runners.Parameterized does not exist > import org.junit.runners.Parameterized.Parameters; > ^ > PaymentRequestPaymentAppAndCardsTest.java:27: error: cannot find symbol > @RunWith(Parameterized.class) > ^ > symbol: class RunWith > PaymentRequestPaymentAppAndCardsTest.java:29: error: cannot find symbol > @Parameters > ^ > symbol: class Parameters > location: class PaymentRequestPaymentAppAndCardsTest > 5 errors Eh, just make a function that takes in the parameters and runs the lines. No need to do the real parameter setup.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dfalcantara@, ptal patch 3. https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java:101: private Comparator<Completable> mCompletenessComparator; On 2016/07/21 22:15:14, dfalcantara wrote: > On 2016/07/21 22:06:23, Rouslan (ツ) wrote: > > On 2016/07/21 21:24:42, dfalcantara wrote: > > > private final? > > > > Cannot be final because it may not be initialized if any of these are true: > > > > (1) webContents == null > > > > (2) ContentViewCore.fromWebContents(webContents) == null > > > > (3) ContentViewCore.fromWebContents(webContents).getWindowAndroid() == null > > > > (4) > > ContentViewCore.fromWebContents(webContents).getWindowAndroid().getActivity() > == > > null > > > > > > Alternatively I can make the variable final if I initialize it inline where > it's > > declared. However, I think that makes the code less readable, because > > initialization is 5 lines. > > Yeah, I figured initializing it where it's declared would make the most sense, > especially because it's not reliant on any of the other things. You could even > make it a > > private static final Comparator<Completable> COMPLETEDNESS_COMPARATOR = new > blah() { > > } > > so that it's shared among all the instances because it never changes. Will > leave it to your judgement, though. Done. https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java (right): https://chromiumcodereview.appspot.com/2165163002/diff/1/chrome/android/javat... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppAndCardsTest.java:92: installPaymentApp(HAVE_INSTRUMENTS, IMMEDIATE_RESPONSE); On 2016/07/21 22:15:14, dfalcantara wrote: > On 2016/07/21 22:06:23, Rouslan (ツ) wrote: > > On 2016/07/21 21:24:42, dfalcantara wrote: > > > are these tests all basically the same? Does it make sense to just have a > > > parameterized runTest function that takes (NO_INSTRUMENTS/HAVE_INSTRUMENTS, > > > IMMEDIATE_RESPONSE/DELAYED_RESPONSE, numberOfPaymentInstruments)? > > > > I can't use the junit parameterization classes in our integration tests. Do we > > have custom classes for this or it's flat out not supported? I assume we don't > > want to do this manually. > > > > PaymentRequestPaymentAppAndCardsTest.java:15: error: package org.junit.runner > > does not exist > > import org.junit.runner.RunWith; > > ^ > > PaymentRequestPaymentAppAndCardsTest.java:16: error: package org.junit.runners > > does not exist > > import org.junit.runners.Parameterized; > > ^ > > PaymentRequestPaymentAppAndCardsTest.java:17: error: package > > org.junit.runners.Parameterized does not exist > > import org.junit.runners.Parameterized.Parameters; > > ^ > > PaymentRequestPaymentAppAndCardsTest.java:27: error: cannot find symbol > > @RunWith(Parameterized.class) > > ^ > > symbol: class RunWith > > PaymentRequestPaymentAppAndCardsTest.java:29: error: cannot find symbol > > @Parameters > > ^ > > symbol: class Parameters > > location: class PaymentRequestPaymentAppAndCardsTest > > 5 errors > > Eh, just make a function that takes in the parameters and runs the lines. No > need to do the real parameter setup. Done.
lgtm
The CQ bit was checked by rouslan@chromium.org
Message was sent while issue was closed.
Description was changed from ========== Autofill cards at the bottom of the PaymentRequest UI This patch arranges the payment instruments in the following order: > Non-autofill payment apps > Complete credit cards from Autofill. > Incomplete credit card from Autofill (need editing before sending to the merchant). BUG=629882 ========== to ========== Autofill cards at the bottom of the PaymentRequest UI This patch arranges the payment instruments in the following order: > Non-autofill payment apps > Complete credit cards from Autofill. > Incomplete credit card from Autofill (need editing before sending to the merchant). BUG=629882 ==========
The CQ bit was checked by rouslan@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.
Description was changed from ========== Autofill cards at the bottom of the PaymentRequest UI This patch arranges the payment instruments in the following order: > Non-autofill payment apps > Complete credit cards from Autofill. > Incomplete credit card from Autofill (need editing before sending to the merchant). BUG=629882 ========== to ========== Autofill cards at the bottom of the PaymentRequest UI This patch arranges the payment instruments in the following order: > Non-autofill payment apps > Complete credit cards from Autofill. > Incomplete credit card from Autofill (need editing before sending to the merchant). BUG=629882 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Autofill cards at the bottom of the PaymentRequest UI This patch arranges the payment instruments in the following order: > Non-autofill payment apps > Complete credit cards from Autofill. > Incomplete credit card from Autofill (need editing before sending to the merchant). BUG=629882 ========== to ========== Autofill cards at the bottom of the PaymentRequest UI This patch arranges the payment instruments in the following order: > Non-autofill payment apps > Complete credit cards from Autofill. > Incomplete credit card from Autofill (need editing before sending to the merchant). BUG=629882 Committed: https://crrev.com/84204fda8113ffef6766812db30d6e11fee1269c Cr-Commit-Position: refs/heads/master@{#407179} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/84204fda8113ffef6766812db30d6e11fee1269c Cr-Commit-Position: refs/heads/master@{#407179} |
