[PRImpl] Move url checks and Delegate of initAndValidate() into CPRImpl

Change:
* Move part of PRImpl#initAndValidate() into CPRImpl#initAndValidate()
* Since the move of initAndValidate() involves
Delegate.getInvalidSslCertificateErrorMessage(), PRImpl.Delegate is
moved into CPRImpl becoming CPRImpl.Delegate.
* Since the move of initAndValidate() involves mPaymentOptions and
mRequestShipping, etc., they are duplicated in
ComponentPaymentRequestImpl. This duplication is necessary because
PRImpl and CPRImpl may lose access to each other, and is without the
out-of-sync issue because mPaymentOptions is final.
* Since the Delegate depends on ChromeActivity, the ChromeActivity
dependencies is refactored to WebContents dependencies.
* Simplifies Delegate#isOffTheRecord(), Delegate#isWebContentsActive(),
Delegate#getTwaPackageName() because they don't need to rely on a
WebContents/ChromeActivity parameter.

Bug: 1102522

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

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

@@ -4,14 +4,11 @@
 
 package org.chromium.chrome.browser.payments;
 
-import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
 import org.chromium.chrome.browser.app.ChromeActivity;
 import org.chromium.chrome.browser.preferences.Pref;
 import org.chromium.chrome.browser.profiles.Profile;
-import org.chromium.chrome.browser.tabmodel.TabModel;
-import org.chromium.chrome.browser.tabmodel.TabModelUtils;
 import org.chromium.components.payments.ComponentPaymentRequestImpl;
 import org.chromium.components.payments.ErrorStrings;
 import org.chromium.components.payments.OriginSecurityChecker;
@@ -20,6 +17,7 @@ import org.chromium.components.payments.SslValidityChecker;
 import org.chromium.components.user_prefs.UserPrefs;
 import org.chromium.content_public.browser.FeaturePolicyFeature;
 import org.chromium.content_public.browser.RenderFrameHost;
+import org.chromium.content_public.browser.Visibility;
 import org.chromium.content_public.browser.WebContents;
 import org.chromium.content_public.browser.WebContentsStatics;
 import org.chromium.mojo.system.MojoException;
@@ -40,7 +38,7 @@ import org.chromium.services.service_manager.InterfaceFactory;
  */
 public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
     // Tests can inject behaviour on future PaymentRequests via these objects.
-    public static PaymentRequestImpl.Delegate sDelegateForTest;
+    public static ComponentPaymentRequestImpl.Delegate sDelegateForTest;
 
     private final RenderFrameHost mRenderFrameHost;
 
@@ -108,8 +106,9 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
      * Production implementation of the PaymentRequestImpl's Delegate. Gives true answers
      * about the system.
      */
-    public static class PaymentRequestDelegateImpl implements PaymentRequestImpl.Delegate {
-        private final TwaPackageManagerDelegate mPackageManager = new TwaPackageManagerDelegate();
+    public static class PaymentRequestDelegateImpl implements ComponentPaymentRequestImpl.Delegate {
+        private final TwaPackageManagerDelegate mPackageManagerDelegate =
+                new TwaPackageManagerDelegate();
         private final RenderFrameHost mRenderFrameHost;
 
         /* package */ PaymentRequestDelegateImpl(RenderFrameHost renderFrameHost) {
@@ -117,39 +116,42 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
         }
 
         @Override
-        public boolean isOffTheRecord(WebContents webContents) {
-            // To be conservative, a request which we don't know its profile is considered
-            // off-the-record, and thus user data would not be recorded in this case.
-            ChromeActivity activity = ChromeActivity.fromWebContents(webContents);
-            if (activity == null) return true;
-            TabModel tabModel = activity.getCurrentTabModel();
-            assert tabModel != null;
-            Profile profile = tabModel.getProfile();
+        public boolean isOffTheRecord() {
+            // TODO(crbug.com/1128658): Try getting around the Profile dependency, as in C++ where
+            // we can do web_contents->GetBrowserContext()->IsOffTheRecord().
+            WebContents liveWebContents = getLiveWebContents();
+            if (liveWebContents == null) return true;
+            Profile profile = Profile.fromWebContents(liveWebContents);
             if (profile == null) return true;
             return profile.isOffTheRecord();
         }
 
         @Override
         public String getInvalidSslCertificateErrorMessage() {
-            WebContents webContents = getWebContents();
-            if (webContents == null || webContents.isDestroyed()) return null;
-            if (!OriginSecurityChecker.isSchemeCryptographic(webContents.getLastCommittedUrl())) {
+            WebContents liveWebContents = getLiveWebContents();
+            if (liveWebContents == null) return null;
+            if (!OriginSecurityChecker.isSchemeCryptographic(
+                        liveWebContents.getLastCommittedUrl())) {
                 return null;
             }
-            return SslValidityChecker.getInvalidSslCertificateErrorMessage(webContents);
+            return SslValidityChecker.getInvalidSslCertificateErrorMessage(liveWebContents);
         }
 
         @Override
-        public boolean isWebContentsActive(@NonNull ChromeActivity activity) {
-            return TabModelUtils.getCurrentWebContents(activity.getCurrentTabModel())
-                    == getWebContents();
+        public boolean isWebContentsActive() {
+            // TODO(crbug.com/1128658): Try making the WebContents inactive for instrumentation
+            // tests rather than mocking it with this method.
+            WebContents liveWebContents = getLiveWebContents();
+            return liveWebContents != null && liveWebContents.getVisibility() == Visibility.VISIBLE;
         }
 
         @Override
         public boolean prefsCanMakePayment() {
-            WebContents webContents = getWebContents();
-            return webContents != null && !webContents.isDestroyed()
-                    && UserPrefs.get(Profile.fromWebContents(webContents))
+            // TODO(crbug.com/1128658): Try replacing Profile with BrowserContextHandle, which
+            // represents a Chrome Profile or WebLayer ProfileImpl, and which UserPrefs operates on.
+            WebContents liveWebContents = getLiveWebContents();
+            return liveWebContents != null
+                    && UserPrefs.get(Profile.fromWebContents(liveWebContents))
                                .getBoolean(Pref.CAN_MAKE_PAYMENT_ENABLED);
         }
 
@@ -160,13 +162,17 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
 
         @Override
         @Nullable
-        public String getTwaPackageName(@Nullable ChromeActivity activity) {
-            return activity != null ? mPackageManager.getTwaPackageName(activity) : null;
+        public String getTwaPackageName() {
+            WebContents liveWebContents = getLiveWebContents();
+            if (liveWebContents == null) return null;
+            ChromeActivity activity = ChromeActivity.fromWebContents(liveWebContents);
+            return activity != null ? mPackageManagerDelegate.getTwaPackageName(activity) : null;
         }
 
         @Nullable
-        private WebContents getWebContents() {
-            return WebContentsStatics.fromRenderFrameHost(mRenderFrameHost);
+        private WebContents getLiveWebContents() {
+            WebContents webContents = WebContentsStatics.fromRenderFrameHost(mRenderFrameHost);
+            return webContents != null && !webContents.isDestroyed() ? webContents : null;
         }
     }
 
@@ -192,7 +198,7 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
             return new InvalidPaymentRequest();
         }
 
-        PaymentRequestImpl.Delegate delegate;
+        ComponentPaymentRequestImpl.Delegate delegate;
         if (sDelegateForTest != null) {
             delegate = sDelegateForTest;
         } else {
@@ -203,8 +209,8 @@ public class PaymentRequestFactory implements InterfaceFactory<PaymentRequest> {
         if (webContents == null || webContents.isDestroyed()) return new InvalidPaymentRequest();
 
         return ComponentPaymentRequestImpl.createPaymentRequest(mRenderFrameHost,
-                /*isOffTheRecord=*/delegate.isOffTheRecord(webContents),
-                /*skipUiForBasicCard=*/delegate.skipUiForBasicCard(),
+                /*isOffTheRecord=*/delegate.isOffTheRecord(),
+                /*skipUiForBasicCard=*/delegate.skipUiForBasicCard(), delegate,
                 (componentPaymentRequest)
                         -> new PaymentRequestImpl(componentPaymentRequest, delegate));
     }

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

@@ -8,7 +8,6 @@ import android.content.Context;
 import android.os.Handler;
 import android.text.TextUtils;
 
-import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 import androidx.annotation.VisibleForTesting;
 import androidx.collection.ArrayMap;
@@ -36,6 +35,7 @@ import org.chromium.components.payments.BrowserPaymentRequest;
 import org.chromium.components.payments.CanMakePaymentQuery;
 import org.chromium.components.payments.CheckoutFunnelStep;
 import org.chromium.components.payments.ComponentPaymentRequestImpl;
+import org.chromium.components.payments.ComponentPaymentRequestImpl.Delegate;
 import org.chromium.components.payments.ErrorMessageUtil;
 import org.chromium.components.payments.ErrorStrings;
 import org.chromium.components.payments.Event;
@@ -98,43 +98,6 @@ public class PaymentRequestImpl
                    PaymentResponseHelper.PaymentResponseRequesterDelegate,
                    PaymentDetailsConverter.MethodChecker, PaymentUIsManager.Delegate,
                    PaymentUIsObserver {
-    /**
-     * A delegate to ask questions about the system, that allows tests to inject behaviour without
-     * having to modify the entire system. This partially mirrors a similar C++
-     * (Content)PaymentRequestDelegate for the C++ implementation, allowing the test harness to
-     * override behaviour in both in a similar fashion.
-     */
-    public interface Delegate {
-        /**
-         * Returns whether the WebContents is currently showing an off-the-record tab. Return true
-         * if the tab profile is not accessible from the WebContents.
-         */
-        boolean isOffTheRecord(WebContents webContents);
-        /**
-         * Returns a non-null string if there is an invalid SSL certificate on the currently
-         * loaded page.
-         */
-        String getInvalidSslCertificateErrorMessage();
-        /**
-         * Returns true if the web contents that initiated the payment request is active.
-         */
-        boolean isWebContentsActive(@NonNull ChromeActivity activity);
-        /**
-         * Returns whether the preferences allow CAN_MAKE_PAYMENT.
-         */
-        boolean prefsCanMakePayment();
-        /**
-         * Returns true if the UI can be skipped for "basic-card" scenarios. This will only ever
-         * be true in tests.
-         */
-        boolean skipUiForBasicCard();
-        /**
-         * If 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(@Nullable ChromeActivity activity);
-    }
 
     private static final String TAG = "PaymentRequest";
     private static boolean sIsLocalCanMakePaymentQueryQuotaEnforcedForTest;
@@ -164,11 +127,13 @@ public class PaymentRequestImpl
 
     private final PaymentUIsManager mPaymentUIsManager;
 
-    private PaymentOptions mPaymentOptions;
-    private boolean mRequestShipping;
-    private boolean mRequestPayerName;
-    private boolean mRequestPayerPhone;
-    private boolean mRequestPayerEmail;
+    @Nullable
+    private final PaymentOptions mPaymentOptions;
+    private final boolean mRequestShipping;
+    private final boolean mRequestPayerName;
+    private final boolean mRequestPayerPhone;
+    private final boolean mRequestPayerEmail;
+    private final int mShippingType;
 
     private boolean mIsCanMakePaymentResponsePending;
     private boolean mIsHasEnrolledInstrumentResponsePending;
@@ -178,7 +143,6 @@ public class PaymentRequestImpl
     private boolean mHasClosed;
 
     private PaymentRequestSpec mSpec;
-    private int mShippingType;
     private boolean mIsFinishedQueryingPaymentApps;
     private List<PaymentApp> mPendingApps = new ArrayList<>();
     private PaymentApp mInvokedPaymentApp;
@@ -263,6 +227,14 @@ public class PaymentRequestImpl
         mWebContents = componentPaymentRequestImpl.getWebContents();
         mMerchantName = mWebContents.getTitle();
         mJourneyLogger = componentPaymentRequestImpl.getJourneyLogger();
+
+        mPaymentOptions = componentPaymentRequestImpl.getPaymentOptions();
+        mRequestShipping = PaymentOptionsUtils.requestShipping(mPaymentOptions);
+        mRequestPayerName = PaymentOptionsUtils.requestPayerName(mPaymentOptions);
+        mRequestPayerPhone = PaymentOptionsUtils.requestPayerPhone(mPaymentOptions);
+        mRequestPayerEmail = PaymentOptionsUtils.requestPayerEmail(mPaymentOptions);
+        mShippingType = PaymentOptionsUtils.getShippingType(mPaymentOptions);
+
         mComponentPaymentRequestImpl = componentPaymentRequestImpl;
         mPaymentUIsManager = new PaymentUIsManager(/*delegate=*/this,
                 /*params=*/this, mWebContents, mIsOffTheRecord, mJourneyLogger, mTopLevelOrigin,
@@ -275,36 +247,6 @@ public class PaymentRequestImpl
     public boolean initAndValidate(PaymentMethodData[] rawMethodData, PaymentDetails details,
             @Nullable PaymentOptions options, boolean googlePayBridgeEligible) {
         assert mComponentPaymentRequestImpl != null;
-        mJourneyLogger.recordCheckoutStep(CheckoutFunnelStep.INITIATED);
-
-        mPaymentOptions = options;
-        mRequestShipping = options != null && options.requestShipping;
-        mRequestPayerName = options != null && options.requestPayerName;
-        mRequestPayerPhone = options != null && options.requestPayerPhone;
-        mRequestPayerEmail = options != null && options.requestPayerEmail;
-        mShippingType = PaymentOptionsUtils.getShippingType(options);
-
-        if (!UrlUtil.isOriginAllowedToUseWebPaymentApis(mWebContents.getLastCommittedUrl())) {
-            Log.d(TAG, ErrorStrings.PROHIBITED_ORIGIN);
-            Log.d(TAG, ErrorStrings.PROHIBITED_ORIGIN_OR_INVALID_SSL_EXPLANATION);
-            mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
-            disconnectFromClientWithDebugMessage(ErrorStrings.PROHIBITED_ORIGIN,
-                    PaymentErrorReason.NOT_SUPPORTED_FOR_INVALID_ORIGIN_OR_SSL);
-            return false;
-        }
-
-        mJourneyLogger.setRequestedInformation(
-                mRequestShipping, mRequestPayerEmail, mRequestPayerPhone, mRequestPayerName);
-
-        String rejectShowErrorMessage = mDelegate.getInvalidSslCertificateErrorMessage();
-        if (!TextUtils.isEmpty(rejectShowErrorMessage)) {
-            Log.d(TAG, rejectShowErrorMessage);
-            Log.d(TAG, ErrorStrings.PROHIBITED_ORIGIN_OR_INVALID_SSL_EXPLANATION);
-            mJourneyLogger.setAborted(AbortReason.INVALID_DATA_FROM_RENDERER);
-            disconnectFromClientWithDebugMessage(rejectShowErrorMessage,
-                    PaymentErrorReason.NOT_SUPPORTED_FOR_INVALID_ORIGIN_OR_SSL);
-            return false;
-        }
 
         boolean googlePayBridgeActivated = googlePayBridgeEligible
                 && SkipToGPayHelper.canActivateExperiment(mWebContents, rawMethodData);
@@ -396,7 +338,7 @@ public class PaymentRequestImpl
     /** @return Whether the UI was built. */
     private boolean buildUI(ChromeActivity activity) {
         String error = mPaymentUIsManager.buildPaymentRequestUI(activity,
-                /*isWebContentsActive=*/mDelegate.isWebContentsActive(activity),
+                /*isWebContentsActive=*/mDelegate.isWebContentsActive(),
                 /*waitForUpdatedDetails=*/mWaitForUpdatedDetails);
         if (error != null) {
             mJourneyLogger.setNotShown(NotShownReason.OTHER);
@@ -1129,15 +1071,15 @@ public class PaymentRequestImpl
         disconnectFromClientWithDebugMessage(ErrorStrings.USER_CANCELLED);
     }
 
+    private void disconnectFromClientWithDebugMessage(String debugMessage) {
+        disconnectFromClientWithDebugMessage(debugMessage, PaymentErrorReason.USER_CANCEL);
+    }
+
     // Implement BrowserPaymentRequest:
     // This method is not supposed to be used outside this class and
     // ComponentPaymentRequestImpl.
     @Override
-    public void disconnectFromClientWithDebugMessage(String debugMessage) {
-        disconnectFromClientWithDebugMessage(debugMessage, PaymentErrorReason.USER_CANCEL);
-    }
-
-    private void disconnectFromClientWithDebugMessage(String debugMessage, int reason) {
+    public void disconnectFromClientWithDebugMessage(String debugMessage, int reason) {
         Log.d(TAG, debugMessage);
         if (mComponentPaymentRequestImpl != null) {
             mComponentPaymentRequestImpl.onError(reason, debugMessage);
@@ -1441,7 +1383,7 @@ public class PaymentRequestImpl
     @Override
     @Nullable
     public String getTwaPackageName() {
-        return mDelegate.getTwaPackageName(ChromeActivity.fromWebContents(mWebContents));
+        return mDelegate.getTwaPackageName();
     }
 
     // PaymentAppFactoryDelegate implementation.

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

@@ -7,12 +7,10 @@ package org.chromium.chrome.test_support;
 import android.os.Build;
 import android.text.TextUtils;
 
-import androidx.annotation.NonNull;
 import androidx.annotation.Nullable;
 
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
-import org.chromium.chrome.browser.app.ChromeActivity;
 import org.chromium.chrome.browser.payments.PaymentRequestFactory;
 import org.chromium.chrome.browser.payments.PaymentRequestImpl;
 import org.chromium.components.autofill.EditableOption;
@@ -33,7 +31,8 @@ public class PaymentRequestTestBridge {
      * about the state of the system, in order to control which paths should be tested in the
      * PaymentRequestImpl.
      */
-    private static class PaymentRequestDelegateForTest implements PaymentRequestImpl.Delegate {
+    private static class PaymentRequestDelegateForTest
+            implements ComponentPaymentRequestImpl.Delegate {
         private final boolean mIsOffTheRecord;
         private final boolean mIsValidSsl;
         private final boolean mIsWebContentsActive;
@@ -50,7 +49,7 @@ public class PaymentRequestTestBridge {
         }
 
         @Override
-        public boolean isOffTheRecord(WebContents webContents) {
+        public boolean isOffTheRecord() {
             return mIsOffTheRecord;
         }
 
@@ -61,7 +60,7 @@ public class PaymentRequestTestBridge {
         }
 
         @Override
-        public boolean isWebContentsActive(@NonNull ChromeActivity activity) {
+        public boolean isWebContentsActive() {
             return mIsWebContentsActive;
         }
 
@@ -77,7 +76,7 @@ public class PaymentRequestTestBridge {
 
         @Override
         @Nullable
-        public String getTwaPackageName(@Nullable ChromeActivity activity) {
+        public String getTwaPackageName() {
             return mTwaPackageName;
         }
     }

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

@@ -7,6 +7,7 @@ package org.chromium.components.payments;
 import androidx.annotation.Nullable;
 
 import org.chromium.payments.mojom.PaymentDetails;
+import org.chromium.payments.mojom.PaymentErrorReason;
 import org.chromium.payments.mojom.PaymentMethodData;
 import org.chromium.payments.mojom.PaymentOptions;
 import org.chromium.payments.mojom.PaymentRequest;
@@ -83,8 +84,12 @@ public interface BrowserPaymentRequest {
     /** The browser part of the {@link PaymentRequest#canMakePayment} implementation. */
     void canMakePayment();
 
-    /** Delegate to the same method of PaymentRequestImpl. */
-    void disconnectFromClientWithDebugMessage(String debugMessage);
+    /**
+     * Delegate to the same method of PaymentRequestImpl.
+     * @param debugMessage The debug message shown for web developers.
+     * @param reason The reason of the disconnection defined in {@link PaymentErrorReason}.
+     */
+    void disconnectFromClientWithDebugMessage(String debugMessage, int reason);
 
     /**
      * Close this instance. The callers of this method should stop referencing this instance upon