Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1952373003:
Use callbacks for testing payments UI. (Closed)
|
Created:
4 years, 7 months ago by please use gerrit instead Modified:
4 years, 7 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. |
DescriptionUse callbacks for testing payments UI.
Instead of polling on the UI thread, let the payments UI notify tests
when it has finished animating and loading data. This should make the
tests less likely to flake.
BUG=608223
Committed: https://crrev.com/1d4cc9c39b0b463d26a51f53e62432ff8b9b2735
Cr-Commit-Position: refs/heads/master@{#392164}
Patch Set 1 #
Total comments: 19
Patch Set 2 : Address comments #
Total comments: 4
Patch Set 3 : Address comments #
Dependent Patchsets: Messages
Total messages: 13 (5 generated)
rouslan@chromium.org changed reviewers: + dfalcantara@chromium.org
dfalcantara@, ptal. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (left): https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:267: mEditButton.setEnabled(false); No need to disable the edit button. onClick() will block interactions and tests are not checking that the buttong is enabled. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:443: if (mPaymentMethodSectionInformation == null) return; I've placed this check into isAcceptingUserInput(). https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:499: // Don't allow any input while the dialog is moving around. This comment was fairly obvious, IMHO. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:178: private boolean mInitialLayoutComplete; getDefaultPaymentInformation() callback may be invoked before the animations have started. This boolean prevents firing "ReadyForInput" callbacks to the test.
https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (left): https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:443: if (mPaymentMethodSectionInformation == null) return; On 2016/05/06 01:45:16, Rouslan wrote: > I've placed this check into isAcceptingUserInput(). This means that they won't be able to dismiss the dialog until the payment information is returned, right? What happens if that call fails to return? https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:499: // Don't allow any input while the dialog is moving around. On 2016/05/06 01:45:16, Rouslan wrote: > This comment was fairly obvious, IMHO. Acknowledged. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:178: private boolean mInitialLayoutComplete; On 2016/05/06 01:45:16, Rouslan wrote: > getDefaultPaymentInformation() callback may be invoked before the animations > have started. This boolean prevents firing "ReadyForInput" callbacks to the > test. mIsInitialLayoutComplete? https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:328: private void notifyObserverForTestReadyForInput() { 1) Don't think we have to be too explicit about the ForTest here, or that we're notifying an Observer. notifyReadyForInput()? 2) Move this to the bottom of the file. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:618: public static void setObserverForTest(PaymentRequestObserverForTest observerForTest) { nit: Move to the bottom because it's a testing method? https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:71: protected ReadyForInputHelper getReadyForInputHelper() { Since this is a test, would it make sense to just make the mReadyForInputHelper a protected final and remove this function? https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:85: mReadyForInputHelper.getUI() I don't think Java style generally allows this kind of one-line-per-call thing. https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:148: assert getCallCount() > 0; I don't think that you need this assert. If the call count is 0, the test would crash/fail anyway, wouldn't it?
Patchset #2 (id:20001) has been deleted
dfalcantara@, ptal patch 2. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (left): https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:443: if (mPaymentMethodSectionInformation == null) return; On 2016/05/06 17:10:33, dfalcantara wrote: > On 2016/05/06 01:45:16, Rouslan wrote: > > I've placed this check into isAcceptingUserInput(). > > This means that they won't be able to dismiss the dialog until the payment > information is returned, right? What happens if that call fails to return? There's still the ◁ button on the phone, but you're right: it's best to keep the X enabled while loading data. Done. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:178: private boolean mInitialLayoutComplete; On 2016/05/06 17:10:32, dfalcantara wrote: > On 2016/05/06 01:45:16, Rouslan wrote: > > getDefaultPaymentInformation() callback may be invoked before the animations > > have started. This boolean prevents firing "ReadyForInput" callbacks to the > > test. > > mIsInitialLayoutComplete? Done. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:328: private void notifyObserverForTestReadyForInput() { On 2016/05/06 17:10:32, dfalcantara wrote: > 1) Don't think we have to be too explicit about the ForTest here, or that we're > notifying > an Observer. > notifyReadyForInput()? > > 2) Move this to the bottom of the file. Done. https://codereview.chromium.org/1952373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:618: public static void setObserverForTest(PaymentRequestObserverForTest observerForTest) { On 2016/05/06 17:10:32, dfalcantara wrote: > nit: Move to the bottom because it's a testing method? Done. https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:71: protected ReadyForInputHelper getReadyForInputHelper() { On 2016/05/06 17:10:33, dfalcantara wrote: > Since this is a test, would it make sense to just make the mReadyForInputHelper > a protected final and remove this function? Done. https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:85: mReadyForInputHelper.getUI() On 2016/05/06 17:10:33, dfalcantara wrote: > I don't think Java style generally allows this kind of one-line-per-call thing. Done. https://codereview.chromium.org/1952373003/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:148: assert getCallCount() > 0; On 2016/05/06 17:10:33, dfalcantara wrote: > I don't think that you need this assert. If the call count is 0, the test would > crash/fail anyway, wouldn't it? Done.
lgtm https://codereview.chromium.org/1952373003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/1952373003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:511: return mCurrentAnimator == null && mPaymentMethodSectionInformation != null nit: This could go out of sync pretty quick. return isAcceptingCloseButton() && mPaymentMethodSectionInformation == null? https://codereview.chromium.org/1952373003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/1952373003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:39: protected final CallbackHelper mDismissed = new CallbackHelper(); protected final above private final
Sending to cq. https://codereview.chromium.org/1952373003/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java (right): https://codereview.chromium.org/1952373003/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java:511: return mCurrentAnimator == null && mPaymentMethodSectionInformation != null On 2016/05/06 20:21:18, dfalcantara wrote: > nit: This could go out of sync pretty quick. > > return isAcceptingCloseButton() && mPaymentMethodSectionInformation == null? Done. https://codereview.chromium.org/1952373003/diff/40001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java (right): https://codereview.chromium.org/1952373003/diff/40001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java:39: protected final CallbackHelper mDismissed = new CallbackHelper(); On 2016/05/06 20:21:18, dfalcantara wrote: > protected final above private final Done.
The CQ bit was checked by rouslan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/1952373003/#ps60001 (title: "Address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1952373003/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1952373003/60001
Message was sent while issue was closed.
Committed patchset #3 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Use callbacks for testing payments UI. Instead of polling on the UI thread, let the payments UI notify tests when it has finished animating and loading data. This should make the tests less likely to flake. BUG=608223 ========== to ========== Use callbacks for testing payments UI. Instead of polling on the UI thread, let the payments UI notify tests when it has finished animating and loading data. This should make the tests less likely to flake. BUG=608223 Committed: https://crrev.com/1d4cc9c39b0b463d26a51f53e62432ff8b9b2735 Cr-Commit-Position: refs/heads/master@{#392164} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/1d4cc9c39b0b463d26a51f53e62432ff8b9b2735 Cr-Commit-Position: refs/heads/master@{#392164} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
