PaymentApp: Implement nativeGetAllAppManifests

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

Index: chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java
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 46cf672f57624db40aa62430df27a7132c158c82..ce59f98d9fb534135896a3e6f7fbf54d5ba7d78d 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
@@ -7,7 +7,6 @@ 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.CalledByNative;
 import org.chromium.base.annotations.SuppressFBWarnings;
 import org.chromium.content_public.browser.WebContents;
@@ -15,7 +14,7 @@ import org.chromium.payments.mojom.PaymentMethodData;
 
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Map;
+import java.util.Set;
 
 /**
  * Native bridge for interacting with service worker based payment apps.
@@ -33,11 +32,11 @@ public class ServiceWorkerPaymentAppBridge implements PaymentAppFactory.PaymentA
      */
     public static class Manifest {
         /**
-         * The scope url of the service worker.
+         * The registration id of the service worker.
          *
          * This can be used to identify a service worker based payment app.
          */
-        public String scopeUrl;
+        public long registrationId;
         public String label;
         public Drawable icon;
         public List<Option> options = new ArrayList<>();
@@ -56,50 +55,33 @@ public class ServiceWorkerPaymentAppBridge implements PaymentAppFactory.PaymentA
         public List<String> enabledMethods = new ArrayList<>();
     }
 
-    /**
-     * 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.
-     */
-    @VisibleForTesting
-    protected List<Manifest> getAllAppManifests() {
-        return nativeGetAllAppManifests();
-    }
-
     @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();
+        nativeGetAllAppManifests(webContents, callback);
     }
 
     /**
      * Invoke a payment app with a given option and matching method data.
+     *
+     * @param webContents    The web contents that invoked PaymentRequest.
+     * @param registrationId The service worker registration if of the Payment App.

[Bernhard Bauer, 2016/12/22 14:03:07]

"[…] service worker registration ID […]"?

[tommyt, 2016/12/22 15:09:35]

That was a silly mistake. Thanks for catching it!

+     * @param optionId       The id of the PaymentOption that was selected by the user.

[Bernhard Bauer, 2016/12/22 14:03:07]

Nit: Capitalize ID.

[tommyt, 2016/12/22 15:09:35]

Done.

+     * @param methodData     The PaymentMethodData objects that are relevant for this payment app.
      */
-    public void invokePaymentapp(
-            String scopeUrl, String optionId, Map<String, PaymentMethodData> methodData) {
-        nativeInvokePaymentApp(scopeUrl, optionId, methodData);
+    public static void invokePaymentApp(WebContents webContents, long registrationId,
+            String optionId, Set<PaymentMethodData> methodData) {
+        nativeInvokePaymentApp(webContents, registrationId, optionId,
+                methodData.toArray(new PaymentMethodData[0]));
     }
 
     @CalledByNative
-    private static List<Manifest> createManifestList() {
-        return new ArrayList<Manifest>();
-    }
-
-    @CalledByNative
-    private static Manifest createAndAddManifest(
-            List<Manifest> manifestList, String scopeUrl, String label, String icon) {
+    private static Manifest createManifest(long registrationId, String label, String icon) {
         Manifest manifest = new Manifest();
-        manifest.scopeUrl = scopeUrl;
+        manifest.registrationId = registrationId;
         manifest.label = label;
         // TODO(tommyt): crbug.com/669876. Handle icons.
         manifest.icon = null;
-        manifestList.add(manifest);
         return manifest;
     }
 
@@ -120,7 +102,21 @@ public class ServiceWorkerPaymentAppBridge implements PaymentAppFactory.PaymentA
         option.enabledMethods.add(enabledMethod);
     }
 
-    private static native List<Manifest> nativeGetAllAppManifests();
-    private static native void nativeInvokePaymentApp(
-            String scopeUrl, String optionId, Object methodDataMap);
+    @CalledByNative
+    private static void onGotManifest(Manifest manifest, WebContents webContents, Object callback) {

[Bernhard Bauer, 2016/12/22 14:03:07]

You can declare |callback| as a PaymentAppFactory.PaymentAppCreatedCallback.

[tommyt, 2016/12/22 15:09:35]

That was my first (and preferred) approach, but if I declare the callback as
PaymentAppFactory.PaymentAppCreatedCallback, the JNI code generator is not
happy, and says:

    SyntaxError: Inner class (PaymentAppFactory.PaymentAppCreatedCallback) can
not be used directly by JNI. Please import the outer class, probably:
    import org.chromium.chrome.browser.payments.PaymentAppFactory;

If I do what this error says to do (namely importing the PaymentAppFactory
class) JNI is happy, and everything works great, but "git cl upload" gives me
the following error when trying to upload the change:

    ** Presubmit ERRORS **
   
chrome/android/java/src/org/chromium/chrome/browser/payments/ServiceWorkerPaymentAppBridge.java:12:1:
Redundant import from the same package -
org.chromium.chrome.browser.payments.PaymentAppFactory.

I can probably sort this out if I spend a little time thinking about it, but if
you have some advice for me on how to best fix this, I would be thankful.

[Bernhard Bauer, 2016/12/22 18:08:42]

Oh, that's annoying. It looks like this is https://crbug.com/505554. One way
around it would be to move the callback to its own class; if you don't want to
do that, can you add a comment here that explains this and references the bug?

[tommyt, 2016/12/23 10:25:49]

I've added a TODO and CC'ed myself on that bug so that I can sort this out once
505554 gets fixed.

+        assert callback instanceof PaymentAppFactory.PaymentAppCreatedCallback;
+        ((PaymentAppFactory.PaymentAppCreatedCallback) callback)
+                .onPaymentAppCreated(new ServiceWorkerPaymentApp(webContents, manifest));
+    }
+
+    @CalledByNative
+    private static void onGotAllManifests(Object callback) {
+        assert callback instanceof PaymentAppFactory.PaymentAppCreatedCallback;
+        ((PaymentAppFactory.PaymentAppCreatedCallback) callback).onAllPaymentAppsCreated();
+    }
+
+    private static native void nativeGetAllAppManifests(WebContents webContents, Object callback);
+
+    private static native void nativeInvokePaymentApp(WebContents webContents, long registrationId,
+            String optionId, PaymentMethodData[] methodData);
 }