[PaymentHandler] Resolve test timeout by correcting PRUI onDismiss

Cause analysis:
We are trying to enable two test suites for BottomSheet(BS)
PaymentHandler (PH) UI - PaymentRequestServiceWorkerPaymentAppTest and
PaymentHandlerChangePaymentMethodTest. Currently the two tests would
fail (BS PH enabled) because they are both timeout on waiting for
PaymentRequestUI#onDismiss() to be called. The reason why it's never
invoked is because when mDialog.isShowing(), not only
mDialog.dismiss() is bypassed (in DimmingDialog), but mDialog's dismiss
listener callback is bypassed as well (in Android's Dialog.java).

Why dimming dialog is not showing in BS PH UI is
because of scrim. For CCT PH UI, the scrim is provided by PR UI; for
BottomSheet PH UI, it's by BottomSheet. As a result, DimmingDialog (the
scrim) is showing in CCT and not showing in BS PH UI.

Before change:
PaymentHandlerChangePaymentMethodTest and
PaymentRequestServiceWorkerPaymentAppTest fails on some tests.

After change:
All tests are green.

Change:
* Replace DialogInterface.onDismissListener with
DimmingDialog.onDismissListener.
* Add onDismissListener invocation for DimmingDialog's dismissal.

Bug: 1045088

Change-Id: Iaedebfb1e9250eeb1974c63083c33cf8a89149df
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2047344
Commit-Queue: Liquan (Max) Gu <maxlg@chromium.org>
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#743142}

chrome/android/chrome_test_java_sources.gni

@@ -311,6 +311,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/password_manager/PasswordManagerDialogTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/AndroidPaymentAppFinderTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/CurrencyFormatterTest.java",
+  "javatests/src/org/chromium/chrome/browser/payments/ExpandablePaymentHandlerChangePaymentMethodTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/MockPackageManagerDelegate.java",
   "javatests/src/org/chromium/chrome/browser/payments/PaymentHandlerChangePaymentMethodTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/PaymentHandlerEnableDelegationsTest.java",
@@ -370,6 +371,7 @@ chrome_test_java_sources = [
   "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestRemoveBillingAddressTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestRetryTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServerCardTest.java",
+  "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerExpandablePaymentHandlerTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressAndOptionTest.java",
   "javatests/src/org/chromium/chrome/browser/payments/PaymentRequestShippingAddressChangeTest.java",

chrome/android/java/src/org/chromium/chrome/browser/payments/ui/DimmingDialog.java

@@ -11,7 +11,6 @@ import android.animation.ObjectAnimator;
 import android.app.Activity;
 import android.app.Dialog;
 import android.content.Context;
-import android.content.DialogInterface;
 import android.graphics.Color;
 import android.graphics.drawable.ColorDrawable;
 import android.os.Build;
@@ -58,16 +57,25 @@ import java.util.Collection;
     private final Dialog mDialog;
     private final ViewGroup mFullContainer;
     private final int mAnimatorTranslation;
+    private OnDismissListener mDismissListener;
     private boolean mIsAnimatingDisappearance;
 
+    /**
+     * Listener for the dismissal of the DimmingDialog.
+     */
+    public interface OnDismissListener {
+        /** Called when the UI is dismissed. */
+        void onDismiss();
+    }
+
     /**
      * Builds the dimming dialog.
      *
      * @param activity        The activity on top of which the dialog should be displayed.
      * @param dismissListener The listener for the dismissal of this dialog.
      */
-    /* package */ DimmingDialog(
-            Activity activity, DialogInterface.OnDismissListener dismissListener) {
+    /* package */ DimmingDialog(Activity activity, OnDismissListener dismissListener) {
+        mDismissListener = dismissListener;
         // To handle the specced animations, the dialog is entirely contained within a translucent
         // FrameLayout. This could eventually be converted to a real BottomSheetDialog, but that
         // requires exploration of how interactions would work when the dialog can be sent back and
@@ -76,7 +84,7 @@ import java.util.Collection;
         mFullContainer.setBackgroundColor(ApiCompatibilityUtils.getColor(
                 activity.getResources(), R.color.modal_dialog_scrim_color));
         mDialog = new AlwaysDismissedDialog(activity, R.style.DimmingDialog);
-        mDialog.setOnDismissListener(dismissListener);
+        mDialog.setOnDismissListener((v) -> notifyListenerDialogDismissed());
         mDialog.addContentView(mFullContainer,
                 new LayoutParams(LayoutParams.MATCH_PARENT, LayoutParams.MATCH_PARENT));
         Window dialogWindow = mDialog.getWindow();
@@ -124,14 +132,20 @@ import java.util.Collection;
      * @param isAnimated If true, the dialog dismissal is animated.
      */
     /* package */ void dismiss(boolean isAnimated) {
-        if (!mDialog.isShowing()) return;
         if (isAnimated) {
             new DisappearingAnimator(true);
         } else {
             mDialog.dismiss();
+            notifyListenerDialogDismissed();
         }
     }
 
+    private void notifyListenerDialogDismissed() {
+        if (mDismissListener == null) return;
+        mDismissListener.onDismiss();
+        mDismissListener = null;
+    }
+
     /** @param overlay The overlay to show. This can be an error dialog, for example. */
     /* package */ void showOverlay(View overlay) {
         // Animate the bottom sheet going away.
@@ -232,7 +246,10 @@ import java.util.Collection;
         public void onAnimationEnd(Animator animation) {
             mIsAnimatingDisappearance = false;
             mFullContainer.removeView(mFullContainer.getChildAt(0));
-            if (mIsDialogClosing && mDialog.isShowing()) mDialog.dismiss();
+            if (mIsDialogClosing) {
+                if (mDialog.isShowing()) mDialog.dismiss();
+                notifyListenerDialogDismissed();
+            }
         }
     }
 

chrome/android/java/src/org/chromium/chrome/browser/payments/ui/PaymentRequestUI.java

@@ -14,7 +14,6 @@ import android.animation.ValueAnimator.AnimatorUpdateListener;
 import android.app.Activity;
 import android.app.Dialog;
 import android.content.Context;
-import android.content.DialogInterface;
 import android.graphics.Bitmap;
 import android.os.Handler;
 import android.support.v4.view.ViewCompat;
@@ -64,8 +63,8 @@ import java.util.List;
 /**
  * The PaymentRequest UI.
  */
-public class PaymentRequestUI implements DialogInterface.OnDismissListener, View.OnClickListener,
-        PaymentRequestSection.SectionDelegate {
+public class PaymentRequestUI implements DimmingDialog.OnDismissListener, View.OnClickListener,
+                                         PaymentRequestSection.SectionDelegate {
     @IntDef({DataType.SHIPPING_ADDRESSES, DataType.SHIPPING_OPTIONS, DataType.CONTACT_DETAILS,
             DataType.PAYMENT_METHODS})
     @Retention(RetentionPolicy.SOURCE)
@@ -876,6 +875,7 @@ public class PaymentRequestUI implements DialogInterface.OnDismissListener, View
     /**
      * Called when user clicks anything in the dialog.
      */
+    // View.OnClickListener implementation.
     @Override
     public void onClick(View v) {
         if (!isAcceptingCloseButton()) return;
@@ -1189,8 +1189,9 @@ public class PaymentRequestUI implements DialogInterface.OnDismissListener, View
      *      window.</li>
      * </ul>
      */
+    // DimmingDialog.OnDismissListener implementation.
     @Override
-    public void onDismiss(DialogInterface dialog) {
+    public void onDismiss() {
         mIsClosing = true;
         if (mEditorDialog.isShowing()) mEditorDialog.dismiss();
         if (mCardEditorDialog.isShowing()) mCardEditorDialog.dismiss();

chrome/android/javatests/src/org/chromium/chrome/browser/payments/ExpandablePaymentHandlerChangePaymentMethodTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.payments;
+
+import android.support.test.InstrumentationRegistry;
+import android.support.test.filters.MediumTest;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.ClassRule;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+import org.chromium.base.test.util.CommandLineFlags;
+import org.chromium.base.test.util.Feature;
+import org.chromium.chrome.R;
+import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
+import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.net.test.EmbeddedTestServer;
+import org.chromium.net.test.ServerCertificate;
+import org.chromium.ui.test.util.DisableAnimationsTestRule;
+
+/** An integration test for PaymentRequestEvent.changePaymentMethod(). */
+@RunWith(ChromeJUnit4ClassRunner.class)
+@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
+        "enable-blink-features=PaymentMethodChangeEvent,PaymentHandlerChangePaymentMethod",
+        "enable-features=" + ChromeFeatureList.SCROLL_TO_EXPAND_PAYMENT_HANDLER})
+@MediumTest
+public class ExpandablePaymentHandlerChangePaymentMethodTest {
+    // Disable animations to reduce flakiness.
+    @ClassRule
+    public static DisableAnimationsTestRule sNoAnimationsRule = new DisableAnimationsTestRule();
+
+    // Open a tab on the blank page first to initiate the native bindings required by the test
+    // server.
+    @Rule
+    public PaymentRequestTestRule mRule = new PaymentRequestTestRule("about:blank");
+
+    // Host the tests on https://127.0.0.1, because file:// URLs cannot have service workers.
+    private EmbeddedTestServer mServer;
+
+    @Before
+    public void setUp() throws Throwable {
+        mServer = EmbeddedTestServer.createAndStartHTTPSServer(
+                InstrumentationRegistry.getContext(), ServerCertificate.CERT_OK);
+        mRule.startMainActivityWithURL(
+                mServer.getURL("/components/test/data/payments/change_payment_method.html"));
+
+        // Find the web contents where JavaScript will be executed and instrument the browser
+        // payment sheet.
+        mRule.openPage();
+    }
+
+    private void installPaymentHandler() throws Throwable {
+        mRule.runJavaScriptCodeInCurrentTab("install();");
+        mRule.expectResultContains(new String[] {"instruments.set(): Payment handler installed."});
+    }
+
+    @After
+    public void tearDown() {
+        mServer.stopAndDestroyServer();
+    }
+
+    /**
+     * Verify that absence of the "paymentmethodchange" event handler in the merchant will cause
+     * PaymentRequestEvent.changePaymentMethod() to resolve with null.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testNoEventHandler() throws Throwable {
+        installPaymentHandler();
+        mRule.clickNodeAndWait("testNoHandler", mRule.getDismissed());
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show(): changePaymentMethod() returned: null"});
+    }
+
+    /**
+     * Verify that absence of the "paymentmethodchange" event handler in the merchant will cause
+     * PaymentRequestEvent.changePaymentMethod() to resolve with null.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testNoEventHandlerBasicCard() throws Throwable {
+        mRule.clickNode("basicCardMethodName");
+        installPaymentHandler();
+        mRule.triggerUIAndWait("testNoHandler", mRule.getReadyToPay());
+        mRule.clickAndWait(R.id.button_primary, mRule.getDismissed());
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show(): changePaymentMethod() returned: null"});
+    }
+
+    /**
+     * Verify that rejecting the promise passed into PaymentMethodChangeEvent.updateWith() will
+     * cause PaymentRequest.show() to reject and thus abort the transaction.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testReject() throws Throwable {
+        installPaymentHandler();
+        mRule.clickNodeAndWait("testReject", mRule.getDismissed());
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show() rejected with: Error for test"});
+    }
+
+    /**
+     * Verify that rejecting the promise passed into PaymentMethodChangeEvent.updateWith() will
+     * cause PaymentRequest.show() to reject and thus abort the transaction.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testRejectBasicCard() throws Throwable {
+        mRule.clickNode("basicCardMethodName");
+        installPaymentHandler();
+        mRule.triggerUIAndWait("testReject", mRule.getReadyToPay());
+        mRule.clickAndWait(R.id.button_primary, mRule.getDismissed());
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show() rejected with: Error for test"});
+    }
+
+    /**
+     * Verify that a JavaScript exception in the "paymentmethodchange" event handler will cause
+     * PaymentRequest.show() to reject and thus abort the transaction.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testThrow() throws Throwable {
+        installPaymentHandler();
+        mRule.clickNodeAndWait("testThrow", mRule.getDismissed());
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show() rejected with: Error: Error for test"});
+    }
+
+    /**
+     * Verify that a JavaScript exception in the "paymentmethodchange" event handler will cause
+     * PaymentRequest.show() to reject and thus abort the transaction.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testThrowBasicCard() throws Throwable {
+        mRule.clickNode("basicCardMethodName");
+        installPaymentHandler();
+        mRule.triggerUIAndWait("testThrow", mRule.getReadyToPay());
+        mRule.clickAndWait(R.id.button_primary, mRule.getDismissed());
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show() rejected with: Error: Error for test"});
+    }
+
+    /**
+     * Verify that the payment handler receives a subset of the payment details passed into
+     * PaymentMethodChangeEvent.updateWith() with URL-based payment method identifier.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testDetails() throws Throwable {
+        installPaymentHandler();
+        mRule.clickNodeAndWait("testDetails", mRule.getDismissed());
+        // Look for the this exact return value to ensure that the browser redacts some details
+        // before forwarding them to the payment handler.
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show(): changePaymentMethod() returned: "
+                                + "{\"error\":\"Error for test\",\"modifiers\":"
+                                + "[{\"data\":{\"soup\":\"potato\"},"
+                                + "\"supportedMethods\":\"https://127.0.0.1:",
+                        // Port changes every time, so don't hardcode it here.
+                        "/pay\",\"total\":{\"amount\":{\"currency\":\"EUR\",\"value\":\"0.03\"},"
+                                + "\"label\":\"\",\"pending\":false}}],"
+                                + "\"paymentMethodErrors\":{\"country\":\"Unsupported country\"},"
+                                + "\"total\":{\"currency\":\"GBP\",\"value\":\"0.02\"}}"});
+    }
+
+    /**
+     * Verify that the payment handler receives a subset of the payment details passed into
+     * PaymentMethodChangeEvent.updateWith() when basic-card payment method is used.
+     */
+    @Test
+    @Feature({"Payments"})
+    public void testDetailsBasicCard() throws Throwable {
+        mRule.clickNode("basicCardMethodName");
+        installPaymentHandler();
+        mRule.triggerUIAndWait("testDetails", mRule.getReadyToPay());
+        mRule.clickAndWait(R.id.button_primary, mRule.getDismissed());
+        // Look for the this exact return value to ensure that the browser redacts some details
+        // before forwarding them to the payment handler.
+        mRule.expectResultContains(
+                new String[] {"PaymentRequest.show(): changePaymentMethod() returned: "
+                        + "{\"error\":\"Error for test\",\"modifiers\":"
+                        + "[{\"data\":{\"soup\":\"potato\"},"
+                        + "\"supportedMethods\":\"basic-card\","
+                        + "\"total\":{\"amount\":{\"currency\":\"EUR\",\"value\":\"0.03\"},"
+                        + "\"label\":\"\",\"pending\":false}}],"
+                        + "\"paymentMethodErrors\":{\"country\":\"Unsupported country\"},"
+                        + "\"total\":{\"currency\":\"GBP\",\"value\":\"0.02\"}}"});
+    }
+}

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentHandlerChangePaymentMethodTest.java

@@ -18,6 +18,7 @@ import org.chromium.base.test.util.CommandLineFlags;
 import org.chromium.base.test.util.Feature;
 import org.chromium.chrome.R;
 import org.chromium.chrome.browser.ChromeSwitches;
+import org.chromium.chrome.browser.flags.ChromeFeatureList;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
 import org.chromium.net.test.EmbeddedTestServer;
 import org.chromium.net.test.ServerCertificate;
@@ -27,7 +28,7 @@ import org.chromium.ui.test.util.DisableAnimationsTestRule;
 @RunWith(ChromeJUnit4ClassRunner.class)
 @CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE,
         "enable-blink-features=PaymentMethodChangeEvent,PaymentHandlerChangePaymentMethod",
-        "disable-field-trial-config"})
+        "disable-features=" + ChromeFeatureList.SCROLL_TO_EXPAND_PAYMENT_HANDLER})
 @MediumTest
 public class PaymentHandlerChangePaymentMethodTest {
     // Disable animations to reduce flakiness.

chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java

@@ -39,9 +39,9 @@ import java.util.concurrent.TimeoutException;
         // For all the tests in this file, we expect abort exception when there is no supported
         // payment apps instead of showing payment request UI.
         "enable-features=" + ChromeFeatureList.STRICT_HAS_ENROLLED_AUTOFILL_INSTRUMENT,
-        "disable-field-trial-config",
         // Prevent crawling the web for real payment apps.
-        "disable-features=" + ChromeFeatureList.SERVICE_WORKER_PAYMENT_APPS})
+        "disable-features=" + ChromeFeatureList.SERVICE_WORKER_PAYMENT_APPS + ","
+                + ChromeFeatureList.SCROLL_TO_EXPAND_PAYMENT_HANDLER})
 public class PaymentRequestServiceWorkerPaymentAppTest {
     // Disable animations to reduce flakiness.
     @ClassRule