Should call onWebContentsForRequestAvailable only once

Change:
* onWebContentsForRequestAvailable() was called two times when
bottomSheetController.requestShowContent() failed in
PaymentHandlerCoordinator. One is in PaymentHandlerCoordinator; one is
in ServiceTabLauncher. Now, onWebContentsForRequestAvailable()
guaranteed to be called only once - in ServiceTabLauncher only.

Bug: 1132972

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

chrome/android/java/src/org/chromium/chrome/browser/ServiceTabLauncher.java

@@ -81,8 +81,12 @@ public class ServiceTabLauncher {
         if (disposition == WindowOpenDisposition.NEW_POPUP) {
             boolean success = false;
             if (PaymentHandlerCoordinator.isEnabled()) {
-                success = PaymentRequestImpl.openPaymentHandlerWindow(url,
-                        (webContents) -> onWebContentsForRequestAvailable(requestId, webContents));
+                WebContents paymentHandlerWebContent =
+                        PaymentRequestImpl.openPaymentHandlerWindow(url);
+                if (paymentHandlerWebContent != null) {
+                    success = true;
+                    onWebContentsForRequestAvailable(requestId, paymentHandlerWebContent);
+                }
             } else {
                 success = createPopupCustomTab(requestId, url.getSpec(), incognito);
             }

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

@@ -20,7 +20,6 @@ import org.chromium.chrome.R;
 import org.chromium.chrome.browser.app.ChromeActivity;
 import org.chromium.chrome.browser.autofill.PersonalDataManager;
 import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator;
-import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator.PaymentHandlerWebContentsObserver;
 import org.chromium.chrome.browser.payments.ui.PaymentInformation;
 import org.chromium.chrome.browser.payments.ui.PaymentRequestUI;
 import org.chromium.chrome.browser.payments.ui.PaymentUIsManager;
@@ -683,39 +682,37 @@ public class PaymentRequestImpl
     /**
      * Called to open a new PaymentHandler UI on the showing PaymentRequest.
      * @param url The url of the payment app to be displayed in the UI.
-     * @param paymentHandlerWebContentsObserver The observer of the WebContents of the
-     * PaymentHandler.
-     * @return Whether the opening is successful.
+     * @return The WebContents of the payment handler that's just opened when the opening is
+     *         successful; null if failed.
      */
-    public static boolean openPaymentHandlerWindow(
-            GURL url, PaymentHandlerWebContentsObserver paymentHandlerWebContentsObserver) {
-        return sShowingPaymentRequest != null
-                && sShowingPaymentRequest.openPaymentHandlerWindowInternal(
-                        url, paymentHandlerWebContentsObserver);
+    @Nullable
+    public static WebContents openPaymentHandlerWindow(GURL url) {
+        if (sShowingPaymentRequest == null) return null;
+        return sShowingPaymentRequest.openPaymentHandlerWindowInternal(url);
     }
 
     /**
      * Called to open a new PaymentHandler UI on this PaymentRequest.
      * @param url The url of the payment app to be displayed in the UI.
-     * @param paymentHandlerWebContentsObserver The observer of the WebContents of the
-     * PaymentHandler.
-     * @return Whether the opening is successful.
+     * @return The WebContents of the payment handler that's just opened when the opening is
+     *         successful; null if failed.
      */
-    private boolean openPaymentHandlerWindowInternal(
-            GURL url, PaymentHandlerWebContentsObserver paymentHandlerWebContentsObserver) {
+    @Nullable
+    private WebContents openPaymentHandlerWindowInternal(GURL url) {
         assert mInvokedPaymentApp != null;
         assert mInvokedPaymentApp.getPaymentAppType() == PaymentAppType.SERVICE_WORKER_APP;
 
-        if (mComponentPaymentRequestImpl == null) return false;
+        if (mComponentPaymentRequestImpl == null) return null;
 
-        boolean success = mPaymentUIsManager.showPaymentHandlerUI(mWebContents, url,
-                paymentHandlerWebContentsObserver, mComponentPaymentRequestImpl.isOffTheRecord());
-        if (success) {
+        @Nullable
+        WebContents paymentHandlerWebContents = mPaymentUIsManager.showPaymentHandlerUI(
+                url, mComponentPaymentRequestImpl.isOffTheRecord());
+        if (paymentHandlerWebContents != null) {
             // UKM for payment app origin should get recorded only when the origin of the invoked
             // payment app is shown to the user.
             mJourneyLogger.setPaymentAppUkmSourceId(mInvokedPaymentApp.getUkmSourceId());
         }
-        return success;
+        return paymentHandlerWebContents;
     }
 
     @Override

chrome/android/java/src/org/chromium/chrome/browser/payments/handler/PaymentHandlerCoordinator.java

@@ -34,7 +34,7 @@ import org.chromium.url.GURL;
  */
 public class PaymentHandlerCoordinator {
     private Runnable mHider;
-    private WebContents mWebContents;
+    private WebContents mPaymentHandlerWebContents;
     private PaymentHandlerToolbarCoordinator mToolbarCoordinator;
 
     /** Constructs the payment-handler component coordinator. */
@@ -50,57 +50,50 @@ public class PaymentHandlerCoordinator {
         void onPaymentHandlerUiShown();
     }
 
-    /** Observes the WebContents of the payment-handler UI. */
-    public interface PaymentHandlerWebContentsObserver {
-        /**
-         * Called when the WebContents has been initialized.
-         * @param webContents The WebContents of the PaymentHandler.
-         */
-        void onWebContentsInitialized(WebContents webContents);
-    }
-
     /**
      * Shows the payment-handler UI.
      *
-     * @param activity The activity where the UI should be shown.
+     * @param paymentRequestWebContents The WebContents of the merchant's frame.
      * @param url The url of the payment handler app, i.e., that of
      *         "PaymentRequestEvent.openWindow(url)".
      * @param isIncognito Whether the tab is in incognito mode.
-     * @param webContentsObserver The observer of the WebContents of the
-     *         PaymentHandler.
      * @param uiObserver The {@link PaymentHandlerUiObserver} that observes this Payment Handler UI.
-     * @return Whether the payment-handler UI was shown. Can be false if the UI was suppressed.
+     * @return The WebContents of the payment handler that's just opened when the showing is
+     *         successful; null if failed.
      */
-    public boolean show(ChromeActivity activity, GURL url, boolean isIncognito,
-            PaymentHandlerWebContentsObserver webContentsObserver,
+    public WebContents show(WebContents paymentRequestWebContents, GURL url, boolean isIncognito,
             PaymentHandlerUiObserver uiObserver) {
         assert mHider == null : "Already showing payment-handler UI";
+        assert paymentRequestWebContents != null;
+        ChromeActivity activity = ChromeActivity.fromWebContents(paymentRequestWebContents);
+        if (activity == null) return null;
 
-        mWebContents = WebContentsFactory.createWebContents(isIncognito, /*initiallyHidden=*/false);
+        mPaymentHandlerWebContents =
+                WebContentsFactory.createWebContents(isIncognito, /*initiallyHidden=*/false);
         ContentView webContentView = ContentView.createContentView(
-                activity, null /* eventOffsetHandler */, mWebContents);
-        initializeWebContents(activity, webContentView, webContentsObserver, url);
+                activity, null /* eventOffsetHandler */, mPaymentHandlerWebContents);
+        initializeWebContents(activity, webContentView, url);
 
-        mToolbarCoordinator = new PaymentHandlerToolbarCoordinator(activity, mWebContents, url);
+        mToolbarCoordinator =
+                new PaymentHandlerToolbarCoordinator(activity, mPaymentHandlerWebContents, url);
 
         PropertyModel model = new PropertyModel.Builder(PaymentHandlerProperties.ALL_KEYS).build();
         PaymentHandlerMediator mediator = new PaymentHandlerMediator(model, this::hide,
-                mWebContents, uiObserver, activity.getActivityTab().getView(),
-                mToolbarCoordinator.getToolbarHeightPx(),
-                activity.getLifecycleDispatcher(),
+                mPaymentHandlerWebContents, uiObserver, activity.getActivityTab().getView(),
+                mToolbarCoordinator.getToolbarHeightPx(), activity.getLifecycleDispatcher(),
                 BottomSheetControllerProvider.from(activity.getWindowAndroid()));
         activity.getWindow().getDecorView().addOnLayoutChangeListener(mediator);
         BottomSheetController bottomSheetController =
                 BottomSheetControllerProvider.from(activity.getWindowAndroid());
         bottomSheetController.addObserver(mediator);
-        mWebContents.addObserver(mediator);
+        mPaymentHandlerWebContents.addObserver(mediator);
 
         mToolbarCoordinator.setCloseButtonOnClickCallback(mediator::onToolbarCloseButtonClicked);
         ThinWebView thinWebView = ThinWebViewFactory.create(activity, new ThinWebViewConstraints());
         assert webContentView.getParent() == null;
-        thinWebView.attachWebContents(mWebContents, webContentView, null);
-        PaymentHandlerView view = new PaymentHandlerView(
-                activity, mWebContents, mToolbarCoordinator.getView(), thinWebView.getView());
+        thinWebView.attachWebContents(mPaymentHandlerWebContents, webContentView, null);
+        PaymentHandlerView view = new PaymentHandlerView(activity, mPaymentHandlerWebContents,
+                mToolbarCoordinator.getView(), thinWebView.getView());
         assert mToolbarCoordinator.getToolbarHeightPx() == view.getToolbarHeightPx();
         PropertyModelChangeProcessor changeProcessor =
                 PropertyModelChangeProcessor.create(model, view, PaymentHandlerViewBinder::bind);
@@ -114,24 +107,27 @@ public class PaymentHandlerCoordinator {
             activity.getWindow().getDecorView().removeOnLayoutChangeListener(mediator);
             mediator.destroy();
             thinWebView.destroy();
-            mWebContents.destroy();
+            mPaymentHandlerWebContents.destroy();
         };
-        return bottomSheetController.requestShowContent(view, /*animate=*/true);
+        boolean isShowSuccess = bottomSheetController.requestShowContent(view, /*animate=*/true);
+        return isShowSuccess ? mPaymentHandlerWebContents : null;
     }
 
-    private void initializeWebContents(ChromeActivity activity, ContentView webContentView,
-            PaymentHandlerWebContentsObserver webContentsObserver, GURL url) {
-        mWebContents.initialize(ChromeVersionInfo.getProductVersion(),
+    private void initializeWebContents(
+            ChromeActivity activity, ContentView webContentView, GURL url) {
+        mPaymentHandlerWebContents.initialize(ChromeVersionInfo.getProductVersion(),
                 ViewAndroidDelegate.createBasicDelegate(webContentView), webContentView,
                 activity.getWindowAndroid(), WebContents.createDefaultInternalsHolder());
 
         SelectionPopupController controller =
-                SelectionPopupController.fromWebContents(mWebContents);
-        controller.setActionModeCallback(new PaymentHandlerActionModeCallback(mWebContents));
-        controller.setSelectionClient(SelectionClient.createSmartSelectionClient(mWebContents));
-
-        webContentsObserver.onWebContentsInitialized(mWebContents);
-        mWebContents.getNavigationController().loadUrl(new LoadUrlParams(url.getSpec()));
+                SelectionPopupController.fromWebContents(mPaymentHandlerWebContents);
+        controller.setActionModeCallback(
+                new PaymentHandlerActionModeCallback(mPaymentHandlerWebContents));
+        controller.setSelectionClient(
+                SelectionClient.createSmartSelectionClient(mPaymentHandlerWebContents));
+
+        mPaymentHandlerWebContents.getNavigationController().loadUrl(
+                new LoadUrlParams(url.getSpec()));
     }
 
     /**
@@ -142,7 +138,7 @@ public class PaymentHandlerCoordinator {
      */
     @VisibleForTesting
     public WebContents getWebContentsForTest() {
-        return mWebContents;
+        return mPaymentHandlerWebContents;
     }
 
     /** Hides the payment-handler UI. */

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

@@ -36,7 +36,6 @@ import org.chromium.chrome.browser.payments.SettingsAutofillAndPaymentsObserver;
 import org.chromium.chrome.browser.payments.ShippingStrings;
 import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator;
 import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator.PaymentHandlerUiObserver;
-import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator.PaymentHandlerWebContentsObserver;
 import org.chromium.chrome.browser.payments.minimal.MinimalUICoordinator;
 import org.chromium.chrome.browser.payments.ui.PaymentRequestSection.OptionSection.FocusChangedObserver;
 import org.chromium.chrome.browser.profiles.Profile;
@@ -941,23 +940,23 @@ public class PaymentUIsManager implements SettingsAutofillAndPaymentsObserver.Ob
 
     /**
      * Create and show the (BottomSheet) PaymentHandler UI.
-     * @param webContents The WebContents of the merchant page.
      * @param url The URL of the payment app.
-     * @param paymentHandlerWebContentsObserver An observer of the WebContents of the Payment
-     *         Handler UI.
      * @param isOffTheRecord Whether the merchant page is currently in an OffTheRecord tab.
-     * @return Whether the PaymentHandler UI is shown successfully.
+     * @return The WebContents of the payment handler that's just opened when the opening is
+     *         successful; null if failed.
      */
-    public boolean showPaymentHandlerUI(WebContents webContents, GURL url,
-            PaymentHandlerWebContentsObserver paymentHandlerWebContentsObserver,
-            boolean isOffTheRecord) {
-        if (mPaymentHandlerUi != null) return false;
-        ChromeActivity chromeActivity = ChromeActivity.fromWebContents(webContents);
-        if (chromeActivity == null) return false;
-
-        mPaymentHandlerUi = new PaymentHandlerCoordinator();
-        return mPaymentHandlerUi.show(chromeActivity, url, isOffTheRecord,
-                paymentHandlerWebContentsObserver, /*uiObserver=*/this);
+    @Nullable
+    public WebContents showPaymentHandlerUI(GURL url, boolean isOffTheRecord) {
+        if (mPaymentHandlerUi != null) return null;
+        ChromeActivity chromeActivity = ChromeActivity.fromWebContents(mWebContents);
+        if (chromeActivity == null) return null;
+
+        PaymentHandlerCoordinator paymentHandlerUi = new PaymentHandlerCoordinator();
+        WebContents paymentHandlerWebContents = paymentHandlerUi.show(
+                /*paymentRequestWebContents=*/mWebContents, url, isOffTheRecord,
+                /*uiObserver=*/this);
+        if (paymentHandlerWebContents != null) mPaymentHandlerUi = paymentHandlerUi;
+        return paymentHandlerWebContents;
     }
 
     @VisibleForTesting(otherwise = VisibleForTesting.NONE)

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

@@ -38,7 +38,6 @@ import org.chromium.chrome.browser.app.ChromeActivity;
 import org.chromium.chrome.browser.flags.ChromeSwitches;
 import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator;
 import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator.PaymentHandlerUiObserver;
-import org.chromium.chrome.browser.payments.handler.PaymentHandlerCoordinator.PaymentHandlerWebContentsObserver;
 import org.chromium.chrome.test.ChromeJUnit4RunnerDelegate;
 import org.chromium.content_public.browser.LoadUrlParams;
 import org.chromium.content_public.browser.WebContents;
@@ -71,7 +70,6 @@ public class ExpandablePaymentHandlerTest {
     private EmbeddedTestServer mServer;
     private boolean mUiShownCalled;
     private boolean mUiClosedCalled;
-    private boolean mWebContentsInitializedCallbackInvoked;
     private UiDevice mDevice;
     private boolean mDefaultIsIncognito;
     private ChromeActivity mDefaultActivity;
@@ -133,8 +131,8 @@ public class ExpandablePaymentHandlerTest {
         PaymentHandlerCoordinator paymentHandler = new PaymentHandlerCoordinator();
         mRule.runOnUiThread(
                 ()
-                        -> paymentHandler.show(mDefaultActivity, defaultPaymentAppUrl(),
-                                isIncognito, defaultWebContentObserver(), defaultUiObserver()));
+                        -> paymentHandler.show(mDefaultActivity.getCurrentWebContents(),
+                                defaultPaymentAppUrl(), isIncognito, defaultUiObserver()));
         return paymentHandler;
     }
 
@@ -161,15 +159,6 @@ public class ExpandablePaymentHandlerTest {
                 "/components/test/data/payments/maxpay.com/payment_handler_window.html"));
     }
 
-    private PaymentHandlerWebContentsObserver defaultWebContentObserver() {
-        return new PaymentHandlerWebContentsObserver() {
-            @Override
-            public void onWebContentsInitialized(WebContents webContents) {
-                mWebContentsInitializedCallbackInvoked = true;
-            }
-        };
-    }
-
     private PaymentHandlerUiObserver defaultUiObserver() {
         return new PaymentHandlerUiObserver() {
             @Override
@@ -247,8 +236,6 @@ public class ExpandablePaymentHandlerTest {
         PaymentHandlerCoordinator paymentHandler = createPaymentHandlerAndShow(mDefaultIsIncognito);
         waitForUiShown();
 
-        Assert.assertTrue(mWebContentsInitializedCallbackInvoked);
-
         mRule.runOnUiThread(() -> paymentHandler.hide());
         waitForUiClosed();
     }