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}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
index 69116d157..a93ef0d 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentAppFactory.java
@@ -11,23 +11,37 @@
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 @@
*
* @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.
+ * @return The singleton PaymentAppFactory instance.
+ */
+ public static PaymentAppFactory getInstance() {
+ if (sInstance == null) sInstance = new PaymentAppFactory();
+ return sInstance;
+ }
+
+ /**
+ * Add an additional factory that can build instances of payment apps.
*
* @param additionalFactory Can build instances of payment apps.
*/
@VisibleForTesting
- public static void setAdditionalFactory(PaymentAppFactoryAddition additionalFactory) {
- sAdditionalFactory = additionalFactory;
- }
-
- /**
- * Set a custom native bridge for service worker payment apps for testing.
- */
- @VisibleForTesting
- public static void setServiceWorkerPaymentAppBridgeForTest(
- ServiceWorkerPaymentAppBridge bridge) {
- sServiceWorkerPaymentAppBridge = bridge;
+ public void addAdditionalFactory(PaymentAppFactoryAddition additionalFactory) {
+ mAdditionalFactories.add(additionalFactory);
}
/**
@@ -66,40 +88,34 @@
*
* @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));
+ public void create(
+ Context context, WebContents webContents, final PaymentAppCreatedCallback callback) {
+ callback.onPaymentAppCreated(new AutofillPaymentApp(context, webContents));
- result.addAll(createServiceWorkerPaymentApps());
-
- if (sAdditionalFactory != null) {
- result.addAll(
- sAdditionalFactory.create(context, webContents));
- }
- return result;
- }
-
- /**
- * 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<>();
+ final Set<PaymentAppFactoryAddition> mPendingTasks =
+ new HashSet<PaymentAppFactoryAddition>(mAdditionalFactories);
- for (ServiceWorkerPaymentAppBridge.Manifest manifest :
- sServiceWorkerPaymentAppBridge.getAllAppManifests()) {
- result.add(new ServiceWorkerPaymentApp(manifest));
+ 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;
}
}
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
index c2ad5659..9d2977d8 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/PaymentRequestImpl.java
@@ -70,9 +70,11 @@
* 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 @@
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 @@
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 @@
}
});
- 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 @@
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 @@
* @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.
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java
index ac430d6..5f25aae 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java
+++ b/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 @@
*/
// 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 @@
}
/**
- * 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();
+ }
}
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppTest.java
index 85ea61b55..e8ce632 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestPaymentAppTest.java
@@ -129,4 +129,34 @@
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"});
+ }
}
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java
index 26f9cb5..9d80712 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestServiceWorkerPaymentAppTest.java
@@ -37,7 +37,7 @@
* ONE_OPTION or TWO_OPTIONS
*/
private void installServiceWorkerPaymentApp(final int instrumentPresence) {
- PaymentAppFactory.setServiceWorkerPaymentAppBridgeForTest(
+ PaymentAppFactory.getInstance().addAdditionalFactory(
new ServiceWorkerPaymentAppBridge() {
@Override
public List<Manifest> getAllAppManifests() {
diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java
index 7612cd0..0507e19 100644
--- a/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java
+++ b/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestTestBase.java
@@ -65,6 +65,12 @@
/** 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 @@
}
@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 @@
protected void openPageAndClickNodeAndWait(String nodeId, CallbackHelper helper)
throws InterruptedException, ExecutionException, TimeoutException {
- startMainActivityWithURL(mTestFilePath);
onMainActivityStarted();
ThreadUtils.runOnUiThreadBlocking(new Runnable() {
@Override
@@ -776,7 +783,7 @@
* @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 @@
* @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() {
+ PaymentAppFactory.getInstance().addAdditionalFactory(new PaymentAppFactoryAddition() {
@Override
- public List<PaymentApp> create(Context context, WebContents webContents) {
- List<PaymentApp> additionalApps = new ArrayList<>();
- additionalApps.add(app);
- return additionalApps;
+ 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;
