Refactor WebApkServiceConnectionManager.

chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java

Index: chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java
diff --git a/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java
new file mode 100644
index 0000000000000000000000000000000000000000..853490d8b0868c9ad7d05a1802b5ed02ff4e81d9
--- /dev/null
+++ b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceConnectionManager.java
@@ -0,0 +1,168 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.webapk.lib.client;
+
+import android.content.ComponentName;
+import android.content.Context;
+import android.content.Intent;
+import android.content.ServiceConnection;
+import android.os.AsyncTask;
+import android.os.IBinder;
+import android.util.Log;
+
+import org.chromium.webapk.lib.common.IdentityService.IIdentityService;
+
+import java.util.ArrayList;
+import java.util.Hashtable;
+
+/**
+ * Manages static global connections between the Chrome application and "WebAPK Identity services".
+ * Each WebAPK has its own "WebAPK Identity Service".
+ */
+public class WebApkIdentityServiceConnectionManager {

[Yaron, 2017/07/10 18:09:07]

shoudl we just merge this into WebApkServiceConnectionManager? The simplest way
would be to use generics, and have one version of this for the chidl service and
another for the identity service. Alternatively, you could just have two types
for connections and one connection manager.

[Xi Han, 2017/07/12 13:49:14]

I would make WebApkServiceConnectionManager be a base class which manages the
connections. Besides, introduce WebApkServiceClient and
WebApkIdentityServiceClient that are used to connect to two services. Each of
the clients are singleton.

+    /**
+     * Interface for getting notified once Chrome is connected to the WebAPK Identity Service.
+     */
+    public interface ConnectionCallback {
+        /**
+         * Called once Chrome is connected to the WebAPK Identity Service.
+         * @param service The WebAPK Identity Service.
+         */
+        void onConnected(IIdentityService service);
+    }
+
+    /** The status of a connection. */
+    public enum ConnectionState { WAITINT_FOR_CONNECTION, CONNECTED, DISCONNECTED }
+
+    /** Managed connection to WebAPK Identity service. */
+    private static class Connection implements ServiceConnection {
+        private WebApkIdentityServiceConnectionManager mConnectionManager;
+
+        /** The state of the connection. */
+        private ConnectionState mState;
+
+        /** Callbacks to call once the connection is established. */
+        private ArrayList<ConnectionCallback> mCallbacks = new ArrayList<ConnectionCallback>();
+
+        /** WebAPK IBinder interface.*/
+        private IIdentityService mIdentityService;
+
+        public ConnectionState getConnectionState() {
+            return mState;
+        }
+
+        public IIdentityService getService() {
+            return mIdentityService;
+        }
+
+        public void addCallback(ConnectionCallback callback) {
+            mCallbacks.add(callback);
+        }
+
+        public Connection(WebApkIdentityServiceConnectionManager manager) {
+            mConnectionManager = manager;
+            mState = ConnectionState.WAITINT_FOR_CONNECTION;
+        }
+
+        @Override
+        public void onServiceDisconnected(ComponentName name) {
+            mState = ConnectionState.DISCONNECTED;
+            mIdentityService = null;
+            notifyAndClear();

[pkotwicz, 2017/07/10 19:06:06]

I don't think that onServiceDisconnected() will be called if
onServiceConnected() has not been previously called

[Xi Han, 2017/07/12 13:49:14]

The goal here is to remove the "unconnected" connection from the cache, and
notify users.
Consider the situation that:
1) Chrome bind to an unbounded WebAPK (which hasn't choose a runtime host). The
IdentityService will return null.
2) go to app setting, force stop the WebAPK (kill the identity service).
3) re-launch the WebAPK and choose Chrome as runtime host.
Next time Chrome connects to the WebAPK's identity service, it will find the
existing connection and calls identityService.getRuntimeHostBrowserPackageName()
and get a remoteException.

+            mConnectionManager.mConnections.remove(name.getPackageName());

[pkotwicz, 2017/07/10 19:06:05]

Can we get rid of the ConnectionState.DISCONNECTED state?

[Xi Han, 2017/07/12 13:49:14]

Done.

+        }
+
+        @Override
+        public void onServiceConnected(ComponentName name, IBinder service) {
+            mState = ConnectionState.CONNECTED;
+            mIdentityService = IIdentityService.Stub.asInterface(service);
+            Log.d(TAG, String.format("Got IIdentityService: %s", mIdentityService));
+            notifyAndClear();
+        }
+
+        /** Notifies all the callers either the connection is established or failed to connect. */
+        private void notifyAndClear() {
+            if (mCallbacks.isEmpty()) return;

[pkotwicz, 2017/07/10 19:06:06]

Nit: The early return is unnecessary given the current implementation

[Xi Han, 2017/07/12 13:49:14]

Removed.

+
+            for (ConnectionCallback callback : mCallbacks) {
+                callback.onConnected(mIdentityService);
+            }
+            mCallbacks.clear();
+        }
+    }
+
+    private static final String CATEGORY_WEBAPK_IDENTITY_SERVICE =
+            "android.intent.category.WEBAPK_IDENTITY_SERVICE_API";
+    private static final String TAG = "cr_WebApk";

[Yaron, 2017/07/10 18:09:07]

different tag

[Xi Han, 2017/07/12 13:49:14]

Done.

+
+    private static WebApkIdentityServiceConnectionManager sInstance;
+
+    /** Mapping of WebAPK package to WebAPK Identity Service connection.*/
+    private Hashtable<String, Connection> mConnections = new Hashtable<String, Connection>();
+
+    public static WebApkIdentityServiceConnectionManager getInstance() {
+        if (sInstance == null) {
+            sInstance = new WebApkIdentityServiceConnectionManager();
+        }
+        return sInstance;
+    }
+
+    /**
+     * Connects Chrome application to WebAPK Identity Service. Can be called from any thread.
+     * @param appContext Application context.
+     * @param webApkPackage WebAPK package to create connection for.
+     * @param callback Callback to call after connection has been established. Called synchronously
+     *                 if the connection is already established.
+     */
+    public void connect(final Context appContext, final String webApkPackage,
+            final ConnectionCallback callback) {

[pkotwicz, 2017/07/10 19:06:06]

The WebApkIdentityService currently only has a single method. Can this method
take a Callback<Boolean> with the boolean argument indicating whether the
runtime host matches or not

Maybe this method can be renamed to checkWebApkRuntimeHost()

[Xi Han, 2017/07/12 13:49:14]

Sounds good! Update the callback, and move the logic to
WebApkIdentityServiceClient.

+        Connection connection = mConnections.get(webApkPackage);
+        if (connection != null) {
+            switch (connection.getConnectionState()) {
+                case WAITINT_FOR_CONNECTION:
+                    connection.addCallback(callback);
+                    return;
+                case CONNECTED:
+                    callback.onConnected(connection.getService());
+                    return;
+                default:
+                    break;
+            }
+        }
+
+        new AsyncTask<Void, Void, Void>() {
+            @Override
+            protected Void doInBackground(Void... params) {
+                Connection newConnection =
+                        new Connection(WebApkIdentityServiceConnectionManager.this);
+                newConnection.addCallback(callback);
+                mConnections.put(webApkPackage, newConnection);
+                Intent intent = createConnectIntent(webApkPackage);
+                try {
+                    appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE);
+                } catch (SecurityException e) {
+                    Log.w(TAG, "Security failed binding.", e);
+                    return null;
+                }
+
+                return null;
+            }
+        }
+                .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);

[pkotwicz, 2017/07/10 19:06:06]

Is the whitespace correct?

[Xi Han, 2017/07/12 13:49:14]

"git cl format" is always suggested this. Removed.

+    }
+
+    WebApkIdentityServiceConnectionManager() {}

[pkotwicz, 2017/07/10 19:06:06]

Did you mean to make the constructor private?

[Xi Han, 2017/07/12 13:49:14]

Done.

+
+    /**
+     * Creates intent to connect to WebAPK Identity service.
+     * @param webApkPackage The package name of the WebAPK to connect to.
+     */
+    private static Intent createConnectIntent(String webApkPackage) {
+        Intent intent = new Intent();
+        intent.addCategory(CATEGORY_WEBAPK_IDENTITY_SERVICE);
+        intent.setPackage(webApkPackage);
+        return intent;
+    }
+}