[WebLayer] Refactor PaymentRequestService#Delegate

Changes:
* Separated PaymentRequestService#Delegate into the common part (will be
shared with WebLayer) and the Chrome part (moved into
ChromePaymentRequestService#Delegate). After the change, the Chrome
part has the TWA and basic-card related logic (skipUiForBasicCard).
* skipUiForBasicCard is used by both java tests and browser tests. The
java tests depends on its being overridden in
PaymentRequestServiceObserverForTest#onPaymentRequestCreated. The
browser test depends on its being overridden in
PaymentRequestTestBridge#PaymentRequestDelegateForTest
#skipUiForBasicCard (which was buggy as it was not set)

Bug: 1142636

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

chrome/android/java/src/org/chromium/chrome/browser/payments/ChromePaymentRequestFactory.java

@@ -5,6 +5,7 @@
 package org.chromium.chrome.browser.payments;
 
 import androidx.annotation.Nullable;
+import androidx.annotation.VisibleForTesting;
 
 import org.chromium.chrome.browser.app.ChromeActivity;
 import org.chromium.chrome.browser.preferences.Pref;
@@ -29,20 +30,35 @@ import org.chromium.services.service_manager.InterfaceFactory;
  */
 public class ChromePaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
     // Tests can inject behaviour on future PaymentRequests via these objects.
-    public static PaymentRequestService.Delegate sDelegateForTest;
-
+    public static ChromePaymentRequestService.Delegate sDelegateForTest;
+    private static ChromePaymentRequestDelegateImplObserverForTest sObserver;
     private final RenderFrameHost mRenderFrameHost;
 
+    /** Observes the {@link ChromePaymentRequestDelegateImpl} for testing. */
+    @VisibleForTesting
+    /* package */ interface ChromePaymentRequestDelegateImplObserverForTest {
+        /**
+         * Called after an instance of {@link ChromePaymentRequestDelegateImpl} has just been
+         * created.
+         * @param delegateImpl The {@link ChromePaymentRequestDelegateImpl}.
+         */
+        void onCreatedChromePaymentRequestDelegateImpl(
+                ChromePaymentRequestDelegateImpl delegateImpl);
+    }
+
     /**
      * Production implementation of the ChromePaymentRequestService's Delegate. Gives true answers
      * about the system.
      */
-    public static class PaymentRequestDelegateImpl implements PaymentRequestService.Delegate {
+    @VisibleForTesting
+    public static class ChromePaymentRequestDelegateImpl
+            implements ChromePaymentRequestService.Delegate {
         private final TwaPackageManagerDelegate mPackageManagerDelegate =
                 new TwaPackageManagerDelegate();
         private final RenderFrameHost mRenderFrameHost;
+        private boolean mSkipUiForBasicCard;
 
-        /* package */ PaymentRequestDelegateImpl(RenderFrameHost renderFrameHost) {
+        private ChromePaymentRequestDelegateImpl(RenderFrameHost renderFrameHost) {
             mRenderFrameHost = renderFrameHost;
         }
 
@@ -88,7 +104,7 @@ public class ChromePaymentRequestFactory implements InterfaceFactory<PaymentRequ
 
         @Override
         public boolean skipUiForBasicCard() {
-            return false; // Only tests do this.
+            return mSkipUiForBasicCard; // Only tests may set it to true.
         }
 
         @Override
@@ -100,6 +116,11 @@ public class ChromePaymentRequestFactory implements InterfaceFactory<PaymentRequ
             return activity != null ? mPackageManagerDelegate.getTwaPackageName(activity) : null;
         }
 
+        @VisibleForTesting
+        public void setSkipUiForBasicCard() {
+            mSkipUiForBasicCard = true;
+        }
+
         @Nullable
         private WebContents getLiveWebContents() {
             WebContents webContents = WebContentsStatics.fromRenderFrameHost(mRenderFrameHost);
@@ -116,6 +137,14 @@ public class ChromePaymentRequestFactory implements InterfaceFactory<PaymentRequ
         mRenderFrameHost = renderFrameHost;
     }
 
+    /** Set an observer for the payment request service, cannot be null. */
+    @VisibleForTesting
+    public static void setChromePaymentRequestDelegateImplObserverForTest(
+            ChromePaymentRequestDelegateImplObserverForTest observer) {
+        assert observer != null;
+        sObserver = observer;
+    }
+
     @Override
     public PaymentRequest createImpl() {
         if (mRenderFrameHost == null) return new InvalidPaymentRequest();
@@ -129,19 +158,21 @@ public class ChromePaymentRequestFactory implements InterfaceFactory<PaymentRequ
             return new InvalidPaymentRequest();
         }
 
-        PaymentRequestService.Delegate delegate;
+        ChromePaymentRequestService.Delegate delegate;
         if (sDelegateForTest != null) {
             delegate = sDelegateForTest;
         } else {
-            delegate = new PaymentRequestDelegateImpl(mRenderFrameHost);
+            ChromePaymentRequestDelegateImpl delegateImpl =
+                    new ChromePaymentRequestDelegateImpl(mRenderFrameHost);
+            sObserver.onCreatedChromePaymentRequestDelegateImpl(/*delegateImpl=*/delegateImpl);
+            delegate = delegateImpl;
         }
 
         WebContents webContents = WebContentsStatics.fromRenderFrameHost(mRenderFrameHost);
         if (webContents == null || webContents.isDestroyed()) return new InvalidPaymentRequest();
 
         return PaymentRequestService.createPaymentRequest(mRenderFrameHost,
-                /*isOffTheRecord=*/delegate.isOffTheRecord(),
-                /*skipUiForBasicCard=*/delegate.skipUiForBasicCard(), delegate,
+                /*isOffTheRecord=*/delegate.isOffTheRecord(), delegate,
                 (paymentRequestService)
                         -> new ChromePaymentRequestService(paymentRequestService, delegate));
     }

chrome/android/java/src/org/chromium/chrome/browser/payments/ChromePaymentRequestService.java

@@ -50,7 +50,6 @@ import org.chromium.components.payments.PaymentFeatureList;
 import org.chromium.components.payments.PaymentHandlerHost;
 import org.chromium.components.payments.PaymentOptionsUtils;
 import org.chromium.components.payments.PaymentRequestService;
-import org.chromium.components.payments.PaymentRequestService.Delegate;
 import org.chromium.components.payments.PaymentRequestSpec;
 import org.chromium.components.payments.PaymentRequestUpdateEventListener;
 import org.chromium.components.payments.PaymentUIsObserver;
@@ -100,6 +99,7 @@ public class ChromePaymentRequestService
                    PaymentUIsObserver {
     private static final String TAG = "PaymentRequest";
     private static boolean sIsLocalCanMakePaymentQueryQuotaEnforcedForTest;
+
     /**
      * Hold the currently showing PaymentRequest. Used to prevent showing more than one
      * PaymentRequest UI per browser process.
@@ -200,6 +200,23 @@ public class ChromePaymentRequestService
     /** A helper to manage the Skip-to-GPay experimental flow. */
     private SkipToGPayHelper mSkipToGPayHelper;
 
+    /** The delegate of this class */
+    public interface Delegate extends PaymentRequestService.Delegate {
+        /**
+         * @return True if the UI can be skipped for "basic-card" scenarios. This will only ever be
+         *         true in tests.
+         */
+        boolean skipUiForBasicCard();
+
+        /**
+         * @return If the merchant's WebContents is running inside of a Trusted Web Activity,
+         *         returns the package name for Trusted Web Activity. Otherwise returns an empty
+         *         string or null.
+         */
+        @Nullable
+        String getTwaPackageName();
+    }
+
     /**
      * Builds the PaymentRequest service implementation.
      *
@@ -412,8 +429,7 @@ public class ChromePaymentRequestService
             // Calculate skip ui and build ui only after all payment apps are ready and
             // request.show() is called.
             mPaymentUiService.calculateWhetherShouldSkipShowingPaymentRequestUi(mIsUserGestureShow,
-                    mURLPaymentMethodIdentifiersSupported,
-                    mPaymentRequestService.skipUiForNonUrlPaymentMethodIdentifiers(),
+                    mURLPaymentMethodIdentifiersSupported, mDelegate.skipUiForBasicCard(),
                     mPaymentOptions);
             if (!buildUI(chromeActivity)) return;
             if (!mPaymentUiService.shouldSkipShowingPaymentRequestUi()
@@ -1570,8 +1586,7 @@ public class ChromePaymentRequestService
             // is determined.
             assert mIsFinishedQueryingPaymentApps;
             mPaymentUiService.calculateWhetherShouldSkipShowingPaymentRequestUi(mIsUserGestureShow,
-                    mURLPaymentMethodIdentifiersSupported,
-                    mPaymentRequestService.skipUiForNonUrlPaymentMethodIdentifiers(),
+                    mURLPaymentMethodIdentifiersSupported, mDelegate.skipUiForBasicCard(),
                     mPaymentOptions);
             if (!buildUI(chromeActivity)) return;
             if (!mPaymentUiService.shouldSkipShowingPaymentRequestUi()

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

@@ -31,6 +31,8 @@ import org.chromium.chrome.browser.autofill.CardUnmaskPrompt;
 import org.chromium.chrome.browser.autofill.CardUnmaskPrompt.CardUnmaskObserverForTest;
 import org.chromium.chrome.browser.autofill.prefeditor.EditorObserverForTest;
 import org.chromium.chrome.browser.autofill.prefeditor.EditorTextField;
+import org.chromium.chrome.browser.payments.ChromePaymentRequestFactory.ChromePaymentRequestDelegateImpl;
+import org.chromium.chrome.browser.payments.ChromePaymentRequestFactory.ChromePaymentRequestDelegateImplObserverForTest;
 import org.chromium.chrome.browser.payments.ui.PaymentRequestSection.OptionSection;
 import org.chromium.chrome.browser.payments.ui.PaymentRequestSection.OptionSection.OptionRow;
 import org.chromium.chrome.browser.payments.ui.PaymentRequestUI;
@@ -73,7 +75,8 @@ import java.util.concurrent.atomic.AtomicReference;
  */
 public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
         implements PaymentRequestObserverForTest, PaymentRequestServiceObserverForTest,
-                   CardUnmaskObserverForTest, EditorObserverForTest {
+                   ChromePaymentRequestDelegateImplObserverForTest, CardUnmaskObserverForTest,
+                   EditorObserverForTest {
     @IntDef({AppPresence.NO_APPS, AppPresence.HAVE_APPS})
     @Retention(RetentionPolicy.SOURCE)
     public @interface AppPresence {
@@ -145,7 +148,7 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
     final CallbackHelper mPaymentResponseReady;
     final CallbackHelper mCompleteReplied;
     final CallbackHelper mRendererClosedMojoConnection;
-    PaymentRequestService mPaymentRequestService;
+    private ChromePaymentRequestDelegateImpl mChromePaymentRequestDelegateImpl;
     PaymentRequestUI mUI;
 
     private final boolean mDelayStartActivity;
@@ -255,6 +258,8 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
             PaymentRequestUI.setEditorObserverForTest(PaymentRequestTestRule.this);
             PaymentRequestUI.setPaymentRequestObserverForTest(PaymentRequestTestRule.this);
             PaymentRequestService.setObserverForTest(PaymentRequestTestRule.this);
+            ChromePaymentRequestFactory.setChromePaymentRequestDelegateImplObserverForTest(
+                    PaymentRequestTestRule.this);
             CardUnmaskPrompt.setObserverForTest(PaymentRequestTestRule.this);
         });
         assertWaitForPageScaleFactorMatch(1);
@@ -1001,7 +1006,7 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
     /** Allows to skip UI into paymenthandler for"basic-card". */
     protected void enableSkipUIForBasicCard() {
         ThreadUtils.runOnUiThreadBlocking(
-                () -> mPaymentRequestService.setSkipUiForNonUrlPaymentMethodIdentifiersForTest());
+                () -> mChromePaymentRequestDelegateImpl.setSkipUiForBasicCard());
     }
 
     @Override
@@ -1059,9 +1064,10 @@ public class PaymentRequestTestRule extends ChromeTabbedActivityTestRule
     }
 
     @Override
-    public void onPaymentRequestCreated(PaymentRequestService paymentRequestService) {
+    public void onCreatedChromePaymentRequestDelegateImpl(
+            ChromePaymentRequestDelegateImpl delegateImpl) {
         ThreadUtils.assertOnUiThread();
-        mPaymentRequestService = paymentRequestService;
+        mChromePaymentRequestDelegateImpl = delegateImpl;
     }
 
     @Override

chrome/test/android/test_support/src/org/chromium/chrome/test_support/PaymentRequestTestBridge.java

@@ -31,20 +31,48 @@ public class PaymentRequestTestBridge {
      * answers about the state of the system, in order to control which paths should be tested in
      * the ChromePaymentRequestService.
      */
+    private static class ChromePaymentRequestDelegateForTest
+            extends PaymentRequestDelegateForTest implements ChromePaymentRequestService.Delegate {
+        private final boolean mSkipUiForBasicCard;
+        private final String mTwaPackageName;
+
+        ChromePaymentRequestDelegateForTest(boolean isOffTheRecord, boolean isValidSsl,
+                boolean isWebContentsActive, boolean prefsCanMakePayment, String twaPackageName,
+                boolean skipUiForBasicCard) {
+            super(isOffTheRecord, isValidSsl, isWebContentsActive, prefsCanMakePayment);
+            mSkipUiForBasicCard = skipUiForBasicCard;
+            mTwaPackageName = twaPackageName;
+        }
+
+        @Override
+        public boolean skipUiForBasicCard() {
+            return mSkipUiForBasicCard;
+        }
+
+        @Override
+        @Nullable
+        public String getTwaPackageName() {
+            return mTwaPackageName;
+        }
+    }
+
+    /**
+     * A test override of the PaymentRequestService's Delegate. Allows tests to control the
+     * answers about the state of the system, in order to control which paths should be tested in
+     * the ChromePaymentRequestService.
+     */
     private static class PaymentRequestDelegateForTest implements PaymentRequestService.Delegate {
         private final boolean mIsOffTheRecord;
         private final boolean mIsValidSsl;
         private final boolean mIsWebContentsActive;
         private final boolean mPrefsCanMakePayment;
-        private final String mTwaPackageName;
 
         PaymentRequestDelegateForTest(boolean isOffTheRecord, boolean isValidSsl,
-                boolean isWebContentsActive, boolean prefsCanMakePayment, String twaPackageName) {
+                boolean isWebContentsActive, boolean prefsCanMakePayment) {
             mIsOffTheRecord = isOffTheRecord;
             mIsValidSsl = isValidSsl;
             mIsWebContentsActive = isWebContentsActive;
             mPrefsCanMakePayment = prefsCanMakePayment;
-            mTwaPackageName = twaPackageName;
         }
 
         @Override
@@ -67,17 +95,6 @@ public class PaymentRequestTestBridge {
         public boolean prefsCanMakePayment() {
             return mPrefsCanMakePayment;
         }
-
-        @Override
-        public boolean skipUiForBasicCard() {
-            return false;
-        }
-
-        @Override
-        @Nullable
-        public String getTwaPackageName() {
-            return mTwaPackageName;
-        }
     }
 
     /**
@@ -196,9 +213,9 @@ public class PaymentRequestTestBridge {
             boolean isValidSsl, boolean isWebContentsActive, boolean prefsCanMakePayment,
             boolean skipUiForBasicCard, String twaPackageName) {
         if (useDelegate) {
-            ChromePaymentRequestFactory.sDelegateForTest =
-                    new PaymentRequestDelegateForTest(isOffTheRecord, isValidSsl,
-                            isWebContentsActive, prefsCanMakePayment, twaPackageName);
+            ChromePaymentRequestFactory.sDelegateForTest = new ChromePaymentRequestDelegateForTest(
+                    isOffTheRecord, isValidSsl, isWebContentsActive, prefsCanMakePayment,
+                    twaPackageName, skipUiForBasicCard);
         } else {
             ChromePaymentRequestFactory.sDelegateForTest = null;
         }

components/payments/content/android/java/src/org/chromium/components/payments/PaymentRequestService.java

@@ -66,7 +66,6 @@ public class PaymentRequestService {
     private final boolean mRequestPayerPhone;
     private final boolean mRequestPayerEmail;
     private final Delegate mDelegate;
-    private boolean mSkipUiForNonUrlPaymentMethodIdentifiers;
     private PaymentRequestLifecycleObserver mPaymentRequestLifecycleObserver;
     private boolean mHasClosed;
 
@@ -124,32 +123,12 @@ public class PaymentRequestService {
          * @return Whether the preferences allow CAN_MAKE_PAYMENT.
          */
         boolean prefsCanMakePayment();
-
-        /**
-         * @return True if the UI can be skipped for "basic-card" scenarios. This will only ever be
-         *         true in tests.
-         */
-        boolean skipUiForBasicCard();
-
-        /**
-         * @return If the merchant's WebContents is running inside of a Trusted Web Activity,
-         *         returns the package name for Trusted Web Activity. Otherwise returns an empty
-         *         string or null.
-         */
-        @Nullable
-        String getTwaPackageName();
     }
 
     /**
      * A test-only observer for the PaymentRequest service implementation.
      */
     public interface PaymentRequestServiceObserverForTest {
-        /**
-         * Called after an instance of {@link PaymentRequestService} has been created.
-         *
-         * @param paymentRequestService The newly created instance of PaymentRequestService.
-         */
-        void onPaymentRequestCreated(PaymentRequestService paymentRequestService);
 
         /**
          * Called when an abort request was denied.
@@ -212,20 +191,18 @@ public class PaymentRequestService {
      * @param isOffTheRecord Whether the merchant page is in a off-the-record (e.g., incognito,
      *         guest mode) Tab.
      * @param delegate The delegate of this class.
-     * @param skipUiForBasicCard True if the PaymentRequest UI should be skipped when the request
-     *         only supports basic-card methods.
      * @param browserPaymentRequestFactory The factory that generates BrowserPaymentRequest.
      * @return The created instance.
      */
     public static PaymentRequest createPaymentRequest(RenderFrameHost renderFrameHost,
-            boolean isOffTheRecord, boolean skipUiForBasicCard, Delegate delegate,
+            boolean isOffTheRecord, Delegate delegate,
             BrowserPaymentRequest.Factory browserPaymentRequestFactory) {
         return new MojoPaymentRequestGateKeeper(
                 (client, methodData, details, options, googlePayBridgeEligible, onClosedListener)
                         -> PaymentRequestService.createIfParamsValid(renderFrameHost,
-                                isOffTheRecord, skipUiForBasicCard, browserPaymentRequestFactory,
-                                client, methodData, details, options, googlePayBridgeEligible,
-                                onClosedListener, delegate));
+                                isOffTheRecord, browserPaymentRequestFactory, client, methodData,
+                                details, options, googlePayBridgeEligible, onClosedListener,
+                                delegate));
     }
 
     /**
@@ -234,8 +211,7 @@ public class PaymentRequestService {
      */
     @Nullable
     private static PaymentRequestService createIfParamsValid(RenderFrameHost renderFrameHost,
-            boolean isOffTheRecord, boolean skipUiForBasicCard,
-            BrowserPaymentRequest.Factory browserPaymentRequestFactory,
+            boolean isOffTheRecord, BrowserPaymentRequest.Factory browserPaymentRequestFactory,
             @Nullable PaymentRequestClient client, @Nullable PaymentMethodData[] methodData,
             @Nullable PaymentDetails details, @Nullable PaymentOptions options,
             boolean googlePayBridgeEligible, Runnable onClosedListener, Delegate delegate) {
@@ -286,10 +262,8 @@ public class PaymentRequestService {
             return null;
         }
 
-        PaymentRequestService instance =
-                new PaymentRequestService(client, renderFrameHost, webContents, journeyLogger,
-                        options, skipUiForBasicCard, isOffTheRecord, onClosedListener, delegate);
-        instance.onCreated();
+        PaymentRequestService instance = new PaymentRequestService(client, renderFrameHost,
+                webContents, journeyLogger, options, isOffTheRecord, onClosedListener, delegate);
         boolean valid = instance.initAndValidate(
                 browserPaymentRequestFactory, methodData, details, googlePayBridgeEligible);
         if (!valid) {
@@ -299,11 +273,6 @@ public class PaymentRequestService {
         return instance;
     }
 
-    private void onCreated() {
-        if (sObserverForTest == null) return;
-        sObserverForTest.onPaymentRequestCreated(this);
-    }
-
     /** Abort the request, used before this class's instantiation. */
     private static void abortBeforeInstantiation(@Nullable PaymentRequestClient client,
             @Nullable JourneyLogger journeyLogger, String debugMessage, int reason) {
@@ -315,8 +284,7 @@ public class PaymentRequestService {
 
     private PaymentRequestService(PaymentRequestClient client, RenderFrameHost renderFrameHost,
             WebContents webContents, JourneyLogger journeyLogger, PaymentOptions options,
-            boolean skipUiForBasicCard, boolean isOffTheRecord, Runnable onClosedListener,
-            Delegate delegate) {
+            boolean isOffTheRecord, Runnable onClosedListener, Delegate delegate) {
         assert client != null;
         assert renderFrameHost != null;
         assert webContents != null;
@@ -344,7 +312,6 @@ public class PaymentRequestService {
         mMerchantName = mWebContents.getTitle();
         mCertificateChain = CertificateChainHelper.getCertificateChain(mWebContents);
         mIsOffTheRecord = isOffTheRecord;
-        mSkipUiForNonUrlPaymentMethodIdentifiers = skipUiForBasicCard;
         mClient = client;
         mJourneyLogger = journeyLogger;
         mOnClosedListener = onClosedListener;
@@ -581,19 +548,6 @@ public class PaymentRequestService {
         sObserverForTest = observerForTest;
     }
 
-    /**
-     * @return True when skip UI is available for non-url based payment method identifiers (e.g.
-     * basic-card).
-     */
-    public boolean skipUiForNonUrlPaymentMethodIdentifiers() {
-        return mSkipUiForNonUrlPaymentMethodIdentifiers;
-    }
-
-    @VisibleForTesting
-    public void setSkipUiForNonUrlPaymentMethodIdentifiersForTest() {
-        mSkipUiForNonUrlPaymentMethodIdentifiers = true;
-    }
-
     /** Invokes {@link PaymentRequestClient.onPaymentMethodChange}. */
     public void onPaymentMethodChange(String methodName, String stringifiedDetails) {
         // Every caller should stop referencing this class once close() is called.