Refactor WebApkServiceConnectionManager.

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

Index: chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java
diff --git a/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java
index 2ef84539ad96a9cd3894c793140645bc647cbfa1..6c2e09f6b371cf2ebfd9fd1c8954251d3a5daa28 100644
--- a/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java
+++ b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkServiceConnectionManager.java
@@ -12,90 +12,100 @@ import android.os.AsyncTask;
 import android.os.IBinder;
 import android.util.Log;
 
-import org.chromium.webapk.lib.runtime_library.IWebApkApi;
-
 import java.util.ArrayList;
 import java.util.Hashtable;
+import java.util.Map;
 
 /**
- * Manages static global connections between the Chrome application and "WebAPK services". Each
- * WebAPK has its own "WebAPK service".
+ * Each WebAPK has several services. This class manages static global connections between the
+ * Chrome application and the "WebAPK services."
  */
 public class WebApkServiceConnectionManager {
     /**
-     * Interface for getting notified once Chrome is connected to the WebAPK service.
+     * Interface for getting notified once Chrome is connected to a WebAPK Service.
      */
     public interface ConnectionCallback {
         /**
          * Called once Chrome is connected to the WebAPK service.
-         * @param api The WebAPK service API.
+         * @param service The WebAPK service.
          */
-        public void onConnected(IWebApkApi api);
+        void onConnected(IBinder service);
     }
 
-    /**
-     * Managed connection to WebAPK service.
-     */
+    /** Managed connection to WebAPK service. */
     private static class Connection implements ServiceConnection {
         /**
          * Whether the connection to the service was established.
+         * Note: this flag cannot be replaced by checking whether iBinder is null, since the iBinder
+         * is null even when a service is connected successfully in Junit tests.
          */
         private boolean mHasConnected;
 
-        /**
-         * Callbacks to call once the connection is established.
-         */
+        /** The connection manager who owns this connection. */
+        private WebApkServiceConnectionManager mConnectionManager;
+
+        /** Callbacks to call once the connection is established. */
         private ArrayList<ConnectionCallback> mCallbacks = new ArrayList<ConnectionCallback>();
 
-        /**
-         * WebAPK IBinder interface.
-         */
-        private IWebApkApi mApi;
+        /** WebAPK IBinder interface.*/
+        private IBinder mBinder;
 
         public boolean hasConnected() {
             return mHasConnected;
         }
 
-        public IWebApkApi getApi() {
-            return mApi;
+        public IBinder getService() {
+            return mBinder;
         }
 
         public void addCallback(ConnectionCallback callback) {
             mCallbacks.add(callback);
         }
 
+        public Connection(WebApkServiceConnectionManager manager) {
+            mConnectionManager = manager;
+        }
+
         @Override
-        public void onServiceDisconnected(ComponentName name) {}
+        public void onServiceDisconnected(ComponentName name) {
+            mHasConnected = false;
+            mBinder = null;
+            if (name != null) {
+                mConnectionManager.onServiceDisconnected(name.getPackageName());
+            }
+            notifyAndClear();
+        }
 
         @Override
         public void onServiceConnected(ComponentName name, IBinder service) {
             mHasConnected = true;
-            mApi = IWebApkApi.Stub.asInterface(service);
-            Log.d(TAG, String.format("Got IWebApkApi: %s", mApi));
+            mBinder = service;
+            Log.d(TAG, String.format("Got IBinder Service: %s", mBinder));
+            notifyAndClear();
+        }
 
+        /** Notifies all the callers either the connection is established or failed to connect. */
+        private void notifyAndClear() {

[pkotwicz, 2017/07/14 23:20:05]

You can inline this method in onServiceConnected() given that this function is
only called from there

[Xi Han, 2017/07/17 20:45:32]

Done.

             for (ConnectionCallback callback : mCallbacks) {
-                callback.onConnected(mApi);
+                callback.onConnected(mBinder);
             }
             mCallbacks.clear();
         }
     }
 
-    private static final String CATEGORY_WEBAPK_API = "android.intent.category.WEBAPK_API";
-
-    private static final String TAG = "cr_WebApk";
+    private static final String TAG = "cr_WebApkService";
 
-    private static WebApkServiceConnectionManager sInstance;
+    /** The category of the service to connect. */
+    private String mCategory;
 
-    /**
-     * Mapping of WebAPK package to WebAPK service connection.
-     */
+    /** Mapping of WebAPK package to WebAPK service connection.*/
     private Hashtable<String, Connection> mConnections = new Hashtable<String, Connection>();
 
-    public static WebApkServiceConnectionManager getInstance() {
-        if (sInstance == null) {
-            sInstance = new WebApkServiceConnectionManager();
-        }
-        return sInstance;
+    /** Called when a WebAPK Service connection is disconnected. */
+    protected void onServiceDisconnected(String webApkName) {
+        // TODO(hanxi): checks whether to kill the corresponding WebApkActivity if the WebAPK's data
+        // is cleared or the WebAPK is uninstalled.
+        mConnections.remove(webApkName);
     }
 
     /**
@@ -103,36 +113,60 @@ public class WebApkServiceConnectionManager {
      * @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) {
         Connection connection = mConnections.get(webApkPackage);
         if (connection != null) {
             if (connection.hasConnected()) {
-                callback.onConnected(connection.getApi());
+                callback.onConnected(connection.getService());
             } else {
                 connection.addCallback(callback);
             }
             return;
         }
 
-        new AsyncTask<Void, Void, Void>() {
+        new AsyncTask<Void, Void, Boolean>() {
             @Override
-            protected Void doInBackground(Void... params) {
-                Connection newConnection = new Connection();
+            protected Boolean doInBackground(Void... params) {
+                Connection newConnection = new Connection(WebApkServiceConnectionManager.this);
                 newConnection.addCallback(callback);
                 Intent intent = createConnectIntent(webApkPackage);
-
+                boolean success = false;
                 try {
-                    appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE);
-
+                    success =
+                            appContext.bindService(intent, newConnection, Context.BIND_AUTO_CREATE);
                 } catch (SecurityException e) {
                     Log.w(TAG, "Security failed binding.", e);
-                    return null;
+                } finally {

[pkotwicz, 2017/07/14 23:20:05]

Does this need to be in a finally{} block?

[Xi Han, 2017/07/17 20:45:32]

Done.

+                    if (success) {
+                        mConnections.put(webApkPackage, newConnection);
+                    }
+                    return success;
                 }
+            }
+
+            @Override
+            protected void onPostExecute(Boolean success) {
+                if (!success) callback.onConnected(null);
+            }
+        }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+    }
 
-                mConnections.put(webApkPackage, newConnection);
+    /**
+     * Disconnect from all of the WebAPK services. Can be called from any thread.
+     * @param appContext The application context.
+     */
+    public void disconnect(final Context appContext) {
+        new AsyncTask<Void, Void, Void>() {
+            @Override
+            protected Void doInBackground(Void... voids) {
+                for (Map.Entry<String, Connection> entry : mConnections.entrySet()) {
+                    Connection connection = entry.getValue();
+                    if (connection == null || !connection.hasConnected()) continue;

[pkotwicz, 2017/07/14 23:20:05]

- Isn't it impossible for |connection| to be null?
- You should call Context#unbindServce() even for services which have not
connected yet (otherwise when the service connects it will stay connected as
long as Chrome is alive)

[Xi Han, 2017/07/17 20:45:32]

Removed this check.
In patch set 8, I updated this check to connection.getService() != null. The
service is only non-null when it is connected.

+                    appContext.unbindService(connection);
+                }
+                mConnections.clear();
                 return null;
             }
         }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
@@ -162,16 +196,17 @@ public class WebApkServiceConnectionManager {
         }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 
-    WebApkServiceConnectionManager() {
+    WebApkServiceConnectionManager(String category) {
+        mCategory = category;
     }
 
     /**
      * Creates intent to connect to WebAPK service.
      * @param webApkPackage The package name of the WebAPK to connect to.
      */
-    private static Intent createConnectIntent(String webApkPackage) {
+    private Intent createConnectIntent(String webApkPackage) {
         Intent intent = new Intent();
-        intent.addCategory(CATEGORY_WEBAPK_API);
+        intent.addCategory(mCategory);
         intent.setPackage(webApkPackage);
         return intent;
     }