PaymentApp: Make the PaymentAppFactory asynchronous

The code for fetching and filtering payment instruments in
PaymentRequestImpl is asynchronous anyway, so this change is not too
intrusive. The main thing is to insert an extra asynchronous step to
populate the mApps list before payment instrument filtering starts.

If we want to take this further, a good next step would be to start
showing apps as they are discovered, instead of waiting until we have
received all the payment apps. It would also be a good thing to
refactor some of this functionality out of the PaymentRequestImpl, as
it is growing quite complex.

Other changes that went into this commit:

* Change the PaymentAppFactory into a singleton, rather than being a
  holder class for static functions.

* Extend the AdditionalPaymentFactory functionality, so that the
  PaymentAppFactory can have many additional factories. This lets us
  make the ServiceWorkerPaymentAppBridge an additional factory, and
  normalize the relationship between the two classes.

* Add two unit tests for testing delayed payment app creation.

BUG=661608

Review-Url: https://codereview.chromium.org/2559153002
Cr-Commit-Position: refs/heads/master@{#437843}

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

@@ -11,23 +11,37 @@ import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.content_public.browser.WebContents;
 
 import java.util.ArrayList;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Set;
 
 /**
  * Builds instances of payment apps.
  */
 public class PaymentAppFactory {
+    private static PaymentAppFactory sInstance;
+
     /**
      * Can be used to build additional types of payment apps without Chrome knowing about their
      * types.
      */
-    private static PaymentAppFactoryAddition sAdditionalFactory;
+    private final List<PaymentAppFactoryAddition> mAdditionalFactories;
 
     /**
-     * Native bridge that can be used to retrieve information about installed service worker
-     * payment apps.
+     * Interface for receiving newly created apps.
      */
-    private static ServiceWorkerPaymentAppBridge sServiceWorkerPaymentAppBridge;
+    public interface PaymentAppCreatedCallback {
+        /**
+         * Called when the factory has create a payment app. This method may be called
+         * zero, one, or many times before the app creation is finished.
+         */
+        void onPaymentAppCreated(PaymentApp paymentApp);
+
+        /**
+         * Called when the factory is finished creating payment apps.
+         */
+        void onAllPaymentAppsCreated();
+    }
 
     /**
      * The interface for additional payment app factories.
@@ -38,27 +52,35 @@ public class PaymentAppFactory {
          *
          * @param context     The application context.
          * @param webContents The web contents that invoked PaymentRequest.
+         * @param callback    The callback to invoke when apps are created.
          */
-        List<PaymentApp> create(Context context, WebContents webContents);
+        void create(Context context, WebContents webContents, PaymentAppCreatedCallback callback);
+    }
+
+    private PaymentAppFactory() {
+        mAdditionalFactories = new ArrayList<>();
+
+        if (ChromeFeatureList.isEnabled(ChromeFeatureList.SERVICE_WORKER_PAYMENT_APPS)) {
+            mAdditionalFactories.add(new ServiceWorkerPaymentAppBridge());
+        }
     }
 
     /**
-     * Sets the additional factory that can build instances of payment apps.
-     *
-     * @param additionalFactory Can build instances of payment apps.
+     * @return The singleton PaymentAppFactory instance.
      */
-    @VisibleForTesting
-    public static void setAdditionalFactory(PaymentAppFactoryAddition additionalFactory) {
-        sAdditionalFactory = additionalFactory;
+    public static PaymentAppFactory getInstance() {
+        if (sInstance == null) sInstance = new PaymentAppFactory();
+        return sInstance;
     }
 
     /**
-     * Set a custom native bridge for service worker payment apps for testing.
+     * Add an additional factory that can build instances of payment apps.
+     *
+     * @param additionalFactory Can build instances of payment apps.
      */
     @VisibleForTesting
-    public static void setServiceWorkerPaymentAppBridgeForTest(
-            ServiceWorkerPaymentAppBridge bridge) {
-        sServiceWorkerPaymentAppBridge = bridge;
+    public void addAdditionalFactory(PaymentAppFactoryAddition additionalFactory) {
+        mAdditionalFactories.add(additionalFactory);
     }
 
     /**
@@ -66,40 +88,34 @@ public class PaymentAppFactory {
      *
      * @param context     The context.
      * @param webContents The web contents where PaymentRequest was invoked.
+     * @param callback    The callback to invoke when apps are created.
      */
-    public static List<PaymentApp> create(Context context, WebContents webContents) {
-        List<PaymentApp> result = new ArrayList<>(2);
-        result.add(new AutofillPaymentApp(context, webContents));
-
-        result.addAll(createServiceWorkerPaymentApps());
-
-        if (sAdditionalFactory != null) {
-            result.addAll(
-                    sAdditionalFactory.create(context, webContents));
-        }
-        return result;
-    }
+    public void create(
+            Context context, WebContents webContents, final PaymentAppCreatedCallback callback) {
+        callback.onPaymentAppCreated(new AutofillPaymentApp(context, webContents));
 
-    /**
-     * Build a ServiceWorkerPaymentApp instance for each installed service
-     * worker based payment app.
-     */
-    private static List<ServiceWorkerPaymentApp> createServiceWorkerPaymentApps() {
-        if (sServiceWorkerPaymentAppBridge == null) {
-            if (ChromeFeatureList.isEnabled(ChromeFeatureList.SERVICE_WORKER_PAYMENT_APPS)) {
-                sServiceWorkerPaymentAppBridge = new ServiceWorkerPaymentAppBridge();
-            } else {
-                return new ArrayList<>();
-            }
+        if (mAdditionalFactories.isEmpty()) {
+            callback.onAllPaymentAppsCreated();
+            return;
         }
 
-        List<ServiceWorkerPaymentApp> result = new ArrayList<>();
-
-        for (ServiceWorkerPaymentAppBridge.Manifest manifest :
-                sServiceWorkerPaymentAppBridge.getAllAppManifests()) {
-            result.add(new ServiceWorkerPaymentApp(manifest));
+        final Set<PaymentAppFactoryAddition> mPendingTasks =
+                new HashSet<PaymentAppFactoryAddition>(mAdditionalFactories);
+
+        for (int i = 0; i < mAdditionalFactories.size(); i++) {
+            final PaymentAppFactoryAddition additionalFactory = mAdditionalFactories.get(i);
+            additionalFactory.create(context, webContents, new PaymentAppCreatedCallback() {
+                @Override
+                public void onPaymentAppCreated(PaymentApp paymentApp) {
+                    callback.onPaymentAppCreated(paymentApp);
+                }
+
+                @Override
+                public void onAllPaymentAppsCreated() {
+                    mPendingTasks.remove(additionalFactory);
+                    if (mPendingTasks.isEmpty()) callback.onAllPaymentAppsCreated();
+                }
+            });
         }
-
-        return result;
     }
 }

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

@@ -70,9 +70,11 @@ import javax.annotation.Nullable;
  * Android implementation of the PaymentRequest service defined in
  * components/payments/payment_request.mojom.
  */
-public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Client,
-        PaymentApp.InstrumentsCallback, PaymentInstrument.InstrumentDetailsCallback,
-        PaymentResponseHelper.PaymentResponseRequesterDelegate, FocusChangedObserver {
+public class PaymentRequestImpl
+        implements PaymentRequest, PaymentRequestUI.Client, PaymentApp.InstrumentsCallback,
+                   PaymentInstrument.InstrumentDetailsCallback,
+                   PaymentAppFactory.PaymentAppCreatedCallback,
+                   PaymentResponseHelper.PaymentResponseRequesterDelegate, FocusChangedObserver {
     /**
      * A test-only observer for the PaymentRequest service implementation.
      */
@@ -202,7 +204,6 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
     private final ChromeActivity mContext;
     private final String mMerchantName;
     private final String mOrigin;
-    private final List<PaymentApp> mApps;
     private final AddressEditor mAddressEditor;
     private final CardEditor mCardEditor;
     private final PaymentRequestJourneyLogger mJourneyLogger = new PaymentRequestJourneyLogger();
@@ -244,6 +245,8 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
     private Map<String, PaymentMethodData> mMethodData;
     private SectionInformation mShippingAddressesSection;
     private SectionInformation mContactSection;
+    private List<PaymentApp> mApps;
+    private boolean mAllAppsCreated;
     private List<PaymentApp> mPendingApps;
     private List<PaymentInstrument> mPendingInstruments;
     private List<PaymentInstrument> mPendingAutofillInstruments;
@@ -298,7 +301,8 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
                     }
                 });
 
-        mApps = PaymentAppFactory.create(mContext, webContents);
+        mApps = new ArrayList<>();
+        PaymentAppFactory.getInstance().create(mContext, webContents, this);
 
         mAddressEditor = new AddressEditor();
         mCardEditor = new CardEditor(webContents, mAddressEditor, sObserverForTest);
@@ -517,8 +521,20 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
         return Collections.unmodifiableMap(result);
     }
 
+    @Override
+    public void onPaymentAppCreated(PaymentApp paymentApp) {
+        mApps.add(paymentApp);
+    }
+
+    @Override
+    public void onAllPaymentAppsCreated() {
+        mAllAppsCreated = true;
+        getMatchingPaymentInstruments();
+    }
+
     /** Queries the installed payment apps for their instruments that merchant supports. */
     private void getMatchingPaymentInstruments() {
+        if (!mAllAppsCreated || mClient == null || mPendingApps != null) return;
         mPendingApps = new ArrayList<>(mApps);
         mPendingInstruments = new ArrayList<>();
         mPendingAutofillInstruments = new ArrayList<>();
@@ -1208,15 +1224,18 @@ public class PaymentRequestImpl implements PaymentRequest, PaymentRequestUI.Clie
      * @return True if no payment methods are supported
      */
     private boolean disconnectIfNoPaymentMethodsSupported() {
-        boolean waitingForPaymentApps = !mPendingApps.isEmpty() || !mPendingInstruments.isEmpty();
+        if (mPendingApps == null || !mPendingApps.isEmpty() || !mPendingInstruments.isEmpty()) {
+            // Waiting for pending apps and instruments.
+            return false;
+        }
+
         boolean foundPaymentMethods = mPaymentMethodsSection != null
                 && !mPaymentMethodsSection.isEmpty();
         boolean userCanAddCreditCard = mMerchantSupportsAutofillPaymentInstruments
                 && !ChromeFeatureList.isEnabled(ChromeFeatureList.NO_CREDIT_CARD_ABORT);
 
         if (!mArePaymentMethodsSupported
-                || (getIsShowing() && !waitingForPaymentApps && !foundPaymentMethods
-                        && !userCanAddCreditCard)) {
+                || (getIsShowing() && !foundPaymentMethods && !userCanAddCreditCard)) {
             // All payment apps have responded, but none of them have instruments. It's possible to
             // add credit cards, but the merchant does not support them either. The payment request
             // must be rejected.

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

@@ -4,9 +4,12 @@
 
 package org.chromium.chrome.browser.payments;
 
+import android.content.Context;
 import android.graphics.drawable.Drawable;
 
+import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.SuppressFBWarnings;
+import org.chromium.content_public.browser.WebContents;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -16,10 +19,9 @@ import java.util.List;
  */
 // TODO(tommyt): crbug.com/669876. Remove these suppressions when we actually
 // start using all of the functionality in this class.
-@SuppressFBWarnings({
-        "UWF_UNWRITTEN_PUBLIC_OR_PROTECTED_FIELD",
-        "UUF_UNUSED_PUBLIC_OR_PROTECTED_FIELD"})
-public class ServiceWorkerPaymentAppBridge {
+@SuppressFBWarnings(
+        {"UWF_UNWRITTEN_PUBLIC_OR_PROTECTED_FIELD", "UUF_UNUSED_PUBLIC_OR_PROTECTED_FIELD"})
+public class ServiceWorkerPaymentAppBridge implements PaymentAppFactory.PaymentAppFactoryAddition {
     /**
      * This class represents a payment app manifest as defined in the Payment
      * App API specification.
@@ -52,10 +54,25 @@ public class ServiceWorkerPaymentAppBridge {
     }
 
     /**
-     * Get a list of all the installed app manifests.
+     * Fetch all the installed service worker app manifests.
+     *
+     * This method is protected so that it can be overridden by tests.
+     *
+     * @return The installed service worker app manifests.
      */
-    public List<Manifest> getAllAppManifests() {
+    @VisibleForTesting
+    protected List<Manifest> getAllAppManifests() {
         // TODO(tommyt): crbug.com/669876. Implement this function.
         return new ArrayList<Manifest>();
     }
+
+    @Override
+    public void create(Context context, WebContents webContents,
+            PaymentAppFactory.PaymentAppCreatedCallback callback) {
+        List<Manifest> manifests = getAllAppManifests();
+        for (int i = 0; i < manifests.size(); i++) {
+            callback.onPaymentAppCreated(new ServiceWorkerPaymentApp(manifests.get(i)));
+        }
+        callback.onAllPaymentAppsCreated();
+    }
 }

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

@@ -129,4 +129,34 @@ public class PaymentRequestPaymentAppTest extends PaymentRequestTestBase {
         clickAndWait(R.id.button_primary, mDismissed);
         expectResultContains(new String[]{"https://bobpay.com", "\"transaction\"", "1337"});
     }
+
+    /**
+     * Test payment with a Bob Pay that is created with a delay, but responds immediately
+     * to getInstruments.
+     */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testPayViaDelayedFastBobPay()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        installPaymentApp(
+                "https://bobpay.com", HAVE_INSTRUMENTS, IMMEDIATE_RESPONSE, DELAYED_CREATION);
+        triggerUIAndWait(mReadyToPay);
+        clickAndWait(R.id.button_primary, mDismissed);
+        expectResultContains(new String[] {"https://bobpay.com", "\"transaction\"", "1337"});
+    }
+
+    /**
+     * Test payment with a Bob Pay that is created with a delay, and responds slowly to
+     * getInstruments.
+     */
+    @MediumTest
+    @Feature({"Payments"})
+    public void testPayViaDelayedSlowBobPay()
+            throws InterruptedException, ExecutionException, TimeoutException {
+        installPaymentApp(
+                "https://bobpay.com", HAVE_INSTRUMENTS, DELAYED_RESPONSE, DELAYED_CREATION);
+        triggerUIAndWait(mReadyToPay);
+        clickAndWait(R.id.button_primary, mDismissed);
+        expectResultContains(new String[] {"https://bobpay.com", "\"transaction\"", "1337"});
+    }
 }

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

@@ -37,7 +37,7 @@ public class PaymentRequestServiceWorkerPaymentAppTest extends PaymentRequestTes
      *                       ONE_OPTION or TWO_OPTIONS
      */
     private void installServiceWorkerPaymentApp(final int instrumentPresence) {
-        PaymentAppFactory.setServiceWorkerPaymentAppBridgeForTest(
+        PaymentAppFactory.getInstance().addAdditionalFactory(
                 new ServiceWorkerPaymentAppBridge() {
                     @Override
                     public List<Manifest> getAllAppManifests() {

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

@@ -65,6 +65,12 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
     /** Flag for installing a slow payment app. */
     protected static final int DELAYED_RESPONSE = 1;
 
+    /** Flag for immediately installing a payment app. */
+    protected static final int IMMEDIATE_CREATION = 0;
+
+    /** Flag for installing a payment app with a delay. */
+    protected static final int DELAYED_CREATION = 1;
+
     /** The expiration month dropdown index for December. */
     protected static final int DECEMBER = 11;
 
@@ -122,7 +128,9 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
     }
 
     @Override
-    public void startMainActivity() throws InterruptedException {}
+    public void startMainActivity() throws InterruptedException {
+        startMainActivityWithURL(mTestFilePath);
+    }
 
     protected abstract void onMainActivityStarted()
             throws InterruptedException, ExecutionException, TimeoutException;
@@ -146,7 +154,6 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
 
     protected void openPageAndClickNodeAndWait(String nodeId, CallbackHelper helper)
             throws InterruptedException, ExecutionException, TimeoutException {
-        startMainActivityWithURL(mTestFilePath);
         onMainActivityStarted();
         ThreadUtils.runOnUiThreadBlocking(new Runnable() {
             @Override
@@ -776,7 +783,7 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
      * @param instrumentPresence Whether the app has any payment instruments. Either NO_INSTRUMENTS
      *                           or HAVE_INSTRUMENTS.
      * @param responseSpeed      How quickly the app will respond to "get instruments" query. Either
-     *                           IMMEDIATE_RESPONSE, DELAYED_RESPONSE, or NO_RESPONSE.
+     *                           IMMEDIATE_RESPONSE or DELAYED_RESPONSE.
      * @return The installed payment app.
      */
     protected TestPay installPaymentApp(final int instrumentPresence, final int responseSpeed) {
@@ -790,18 +797,45 @@ abstract class PaymentRequestTestBase extends ChromeActivityTestCaseBase<ChromeT
      * @param instrumentPresence Whether the app has any payment instruments. Either NO_INSTRUMENTS
      *                           or HAVE_INSTRUMENTS.
      * @param responseSpeed      How quickly the app will respond to "get instruments" query. Either
-     *                           IMMEDIATE_RESPONSE, DELAYED_RESPONSE, or NO_RESPONSE.
+     *                           IMMEDIATE_RESPONSE or DELAYED_RESPONSE.
      * @return The installed payment app.
      */
     protected TestPay installPaymentApp(final String methodName, final int instrumentPresence,
             final int responseSpeed) {
+        return installPaymentApp(methodName, instrumentPresence, responseSpeed, IMMEDIATE_CREATION);
+    }
+
+    /**
+     * Installs a payment app for testing.
+     *
+     * @param methodName         The name of the payment method used in the payment app.
+     * @param instrumentPresence Whether the app has any payment instruments. Either NO_INSTRUMENTS
+     *                           or HAVE_INSTRUMENTS.
+     * @param responseSpeed      How quickly the app will respond to "get instruments" query. Either
+     *                           IMMEDIATE_RESPONSE or DELAYED_RESPONSE.
+     * @param creationSpeed      How quickly the app factory will create this app. Either
+     *                           IMMEDIATE_CREATION or DELAYED_CREATION.
+     * @return The installed payment app.
+     */
+    protected TestPay installPaymentApp(final String methodName, final int instrumentPresence,
+            final int responseSpeed, final int creationSpeed) {
         final TestPay app = new TestPay(methodName, instrumentPresence, responseSpeed);
-        PaymentAppFactory.setAdditionalFactory(new PaymentAppFactoryAddition() {
-            @Override
-            public List<PaymentApp> create(Context context, WebContents webContents) {
-                List<PaymentApp> additionalApps = new ArrayList<>();
-                additionalApps.add(app);
-                return additionalApps;
+        PaymentAppFactory.getInstance().addAdditionalFactory(new PaymentAppFactoryAddition() {
+            @Override
+            public void create(Context context, WebContents webContents,
+                    final PaymentAppFactory.PaymentAppCreatedCallback callback) {
+                if (creationSpeed == IMMEDIATE_CREATION) {
+                    callback.onPaymentAppCreated(app);
+                    callback.onAllPaymentAppsCreated();
+                } else {
+                    new Handler().postDelayed(new Runnable() {
+                        @Override
+                        public void run() {
+                            callback.onPaymentAppCreated(app);
+                            callback.onAllPaymentAppsCreated();
+                        }
+                    }, 100);
+                }
             }
         });
         return app;