Implement IsReadyToPay handling

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

Index: chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java b/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
index 9850688e84dc2759c5ce39faee259253992124e1..9b33c13a631f378eb963889bf91ec8b37a7b0cab 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/payments/AndroidPaymentApp.java
@@ -5,12 +5,21 @@
 package org.chromium.chrome.browser.payments;
 
 import android.app.Activity;
+import android.content.ComponentName;
+import android.content.Context;
 import android.content.Intent;
+import android.content.ServiceConnection;
 import android.graphics.drawable.Drawable;
+import android.os.BadParcelableException;
 import android.os.Bundle;
 import android.os.Handler;
+import android.os.IBinder;
+import android.os.NetworkOnMainThreadException;
+import android.os.RemoteException;
 import android.util.JsonWriter;
 
+import org.chromium.IsReadyToPayService;
+import org.chromium.IsReadyToPayServiceCallback;
 import org.chromium.chrome.R;
 import org.chromium.content.browser.ContentViewCore;
 import org.chromium.content_public.browser.WebContents;
@@ -42,11 +51,29 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
 
     private final Handler mHandler;
     private final WebContents mWebContents;
+    private final Intent mIsReadyToPayIntent;
     private final Intent mPayIntent;
     private final Set<String> mMethodNames;
-    private String mIsReadyToPayService;
+    private IsReadyToPayService mIsReadyToPayService;
+    private InstrumentsCallback mInstrumentsCallback;
     private InstrumentDetailsCallback mInstrumentDetailsCallback;
 
+    private final ServiceConnection mServiceConnection = new ServiceConnection() {

[please use gerrit instead, 2017/01/13 20:54:46]

Please group all "private final" variables together.

+        @Override
+        public void onServiceConnected(ComponentName name, IBinder service) {
+            mIsReadyToPayService = IsReadyToPayService.Stub.asInterface(service);
+            if (mIsReadyToPayService == null) {
+                sendInstrumentsReady(null);
+            } else {
+                sendIsReadyToPay();

[please use gerrit instead, 2017/01/13 20:54:46]

These send* can get confusing. sendInstrumentsReady() talks to
PaymentRequestImpl, but sendIsReadyToPay() talks to the payment app. Please
specify the destination of the message in the method name for send* methods. For
example, sendIsReadyToPayIntentToPaymentApp(). As for sendInstrumentsReady(),
you may want to rename it into respondToGetInstrumentsQuery(). I think it should
be clear that PaymentRequestImpl is querying for instruments.

+            }
+        }
+
+        @Override
+        public void onServiceDisconnected(ComponentName name) {
+            sendInstrumentsReady(null);
+        }
+    };
     /**
      * Builds the point of interaction with a locally installed 3rd party native Android payment
      * app.
@@ -63,6 +90,8 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
         mHandler = new Handler();
         mWebContents = webContents;
         mPayIntent = new Intent();
+        mIsReadyToPayIntent = new Intent();
+        mIsReadyToPayIntent.setPackage(packageName);
         mPayIntent.setClassName(packageName, activity);
         mPayIntent.setAction(ACTION_PAY);
         mMethodNames = new HashSet<>();
@@ -73,22 +102,25 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
         mMethodNames.add(methodName);
     }
 
-    /** @param service The name of the "is ready to pay" service in the payment app. */
-    public void setIsReadyToPayService(String service) {
-        mIsReadyToPayService = service;
+    /** @param className The class name of the "is ready to pay" service in the payment app. */
+    public void setIsReadyToPayAction(String className) {
+        mIsReadyToPayIntent.setClassName(mIsReadyToPayIntent.getPackage(), className);
     }
 
     @Override
-    public void getInstruments(Map<String, PaymentMethodData> methodData, String origin,
+    public void getInstruments(final Map<String, PaymentMethodData> methodData, final String origin,

[please use gerrit instead, 2017/01/13 20:54:46]

No need for these three "final" keywords, because mHandler is not using these
variables.

             final InstrumentsCallback callback) {
-        mHandler.post(new Runnable() {
-            @Override
-            public void run() {
-                List<PaymentInstrument> instruments = new ArrayList<>();
-                instruments.add(AndroidPaymentApp.this);
-                callback.onInstrumentsReady(AndroidPaymentApp.this, instruments);
-            }
-        });
+        mInstrumentsCallback = callback;
+        if (mIsReadyToPayIntent.getComponent() != null) {

[please use gerrit instead, 2017/01/13 20:54:46]

When you find an IS_READY_TO_PAY service, you call setClassName(), but here you
check getComponent(). This asymmetry is jarring. Can you check getClassName()
here as well?

+            isReadyToPay(origin, methodData.get(mMethodNames.iterator().next()));
+        } else {
+            mHandler.post(new Runnable() {
+                @Override
+                public void run() {
+                    sendInstrumentsReady(AndroidPaymentApp.this);
+                }
+            });
+        }
     }
 
     @Override
@@ -99,6 +131,69 @@ public class AndroidPaymentApp extends PaymentInstrument implements PaymentApp,
         return !methodNames.isEmpty();
     }
 
+    private void sendInstrumentsReady(PaymentInstrument instrument) {
+        List<PaymentInstrument> instruments = null;
+        if (instrument != null) {
+            instruments = new ArrayList<>();
+            instruments.add(instrument);
+        }
+        mInstrumentsCallback.onInstrumentsReady(this, instruments);
+    }
+
+    private void isReadyToPay(String origin, PaymentMethodData data) {

[please use gerrit instead, 2017/01/13 20:54:46]

You can combine this function with getIntsruments(), because it's only called
once:

void getInstruments(methodData, origin, callback) {
  mInstrumentsCallback = callback;
  if (mIsReadyToPayIntent.getClassName() == null) {
    mHandler.post(new Runnable() {
      @Override
      public void run() {
        sendInstrumentsReady(AndroidPaymentApp.this);
      }
    });
    return;
  }
  // Put contents of isReadyToPay(origin, data) here.
}

+        Bundle extras = new Bundle();
+        extras.putString(EXTRA_METHOD_NAME, mMethodNames.iterator().next());
+        extras.putString(EXTRA_ORIGIN, origin);
+        extras.putString(EXTRA_DATA, data == null ? EMPTY_JSON_DATA : data.stringifiedData);
+        mIsReadyToPayIntent.putExtras(extras);
+
+        if (mIsReadyToPayService != null) {
+            sendIsReadyToPay();
+        } else {
+            ContentViewCore contentView = ContentViewCore.fromWebContents(mWebContents);
+            if (contentView == null) {
+                notifyError();
+                return;
+            }
+
+            WindowAndroid window = contentView.getWindowAndroid();
+            if (window == null) {
+                notifyError();
+                return;
+            }
+
+            try {
+                window.getApplicationContext().bindService(
+                        mIsReadyToPayIntent, mServiceConnection, Context.BIND_AUTO_CREATE);
+            } catch (SecurityException e) {
+                sendInstrumentsReady(null);
+            }
+        }
+    }
+
+    private void sendIsReadyToPay() {
+        assert mIsReadyToPayService != null;
+        IsReadyToPayServiceCallback.Stub callback = new IsReadyToPayServiceCallback.Stub() {
+            @Override
+            public void handleIsReadyToPay(boolean isReadyToPay) throws RemoteException {
+                if (isReadyToPay) {
+                    sendInstrumentsReady(AndroidPaymentApp.this);
+                } else {
+                    sendInstrumentsReady(null);
+                }
+            }
+        };
+        try {
+            mIsReadyToPayService.isReadyToPay(callback);
+        } catch (RemoteException | SecurityException | BadParcelableException
+                | IllegalArgumentException | NullPointerException | IllegalStateException
+                | NetworkOnMainThreadException | UnsupportedOperationException e) {

[please use gerrit instead, 2017/01/13 20:54:46]

At this point I think it's OK to use Throwable.

[rwlbuis, 2017/01/13 22:12:25]

Do you mean instead of last two or everyhing? Is this to make it future proof,
i.e if more Exceptions gets thrown we'll cattch them here?

[please use gerrit instead, 2017/01/16 17:41:18]

Instead of everything. It's to make the code more clear. If you have 8
undocumented exceptions to worry about, you might as well handle all of them.

[rwlbuis, 2017/01/17 20:00:35]

Done.

+            /** The exceptions above are not caught in the remote Service but passed on to the

[please use gerrit instead, 2017/01/13 20:54:46]

"The exceptions above" --> "Many undocumented exceptions"

+                Service caller, see writeException in Parcel.java. */
+            sendInstrumentsReady(null);
+        }
+    }
+
     @Override
     public String getAppIdentifier() {
         return getIdentifier();