Refactor WebApkServiceConnectionManager.

chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java

Index: chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java
similarity index 68%
rename from chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java
rename to chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java
index 7d34867a787b8048f9b15b0f7950fb8367c612a8..80b6d4b1c9a688611dcff51abe0003b570bda493 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/notifications/WebApkNotificationClient.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebApkServiceClient.java
@@ -1,45 +1,65 @@
-// Copyright 2016 The Chromium Authors. All rights reserved.
+// 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.chrome.browser.notifications;
+package org.chromium.chrome.browser.webapps;
 
+import android.content.Context;
 import android.content.pm.PackageManager;
 import android.content.res.Resources;
 import android.graphics.Bitmap;
 import android.graphics.BitmapFactory;
 import android.os.Build;
+import android.os.IBinder;
 import android.os.RemoteException;
 
 import org.chromium.base.ContextUtils;
 import org.chromium.base.Log;
+import org.chromium.chrome.browser.notifications.NotificationBuilderBase;
 import org.chromium.webapk.lib.client.WebApkServiceConnectionManager;
 import org.chromium.webapk.lib.runtime_library.IWebApkApi;
 
 /**
- * WebApkNotificationClient provides an API to display and close notifications remotely in
- * context of a WebAPK, enriching the notification with the WebAPK's small icon when available.
+ * Provides APIs that allow browsers communicating with WebAPK services. Each WebAPK has its own
+ * "WebAPK service".

[pkotwicz, 2017/07/19 16:09:13]

Thank you for merging WebApkServiceClient and WebApkNotificationClient. Merging
the two classes makes the implementation much cleaner!

How about: "that allow browsers communicating with" -> "for browsers to
communicate with"

Your sentence is an incomplete sentence
"Provides APIs that allow browsers communicating with WebAPK services to do
something special" is a complete sentence
"Provides APIs that allow browsers to do something special" is a complete
sentence
I can't think of a rule for why it is this way unfortunately.

When I wrote a technical report in university, I got in trouble for using
"allow" and "enable" interchangeably. "allow" and "enable" should not be used
interchangeably as described in this article
https://letlucyedit.wordpress.com/2013/04/03/enable-or-allow-why-you-need-to-...

[Xi Han, 2017/07/21 20:36:34]

Good article! I haven't thought about this before.

  */
-public class WebApkNotificationClient {
-    private static final String TAG = "cr_WebApk";
-
+public class WebApkServiceClient {
     // Callback which catches RemoteExceptions thrown due to IWebApkApi failure.
-    private abstract static class ApiUseCallback
+    public abstract static class ApiUseCallback

[pkotwicz, 2017/07/19 16:09:13]

This can now be private?

[Xi Han, 2017/07/21 20:36:34]

Done.

             implements WebApkServiceConnectionManager.ConnectionCallback {
         public abstract void useApi(IWebApkApi api) throws RemoteException;
 
         @Override
-        public void onConnected(IWebApkApi api) {
+        public void onConnected(IBinder api) {
             try {
-                useApi(api);
+                useApi(IWebApkApi.Stub.asInterface(api));
             } catch (RemoteException e) {
                 Log.w(TAG, "WebApkAPI use failed.", e);
             }
         }
     }
 
+    private static final String CATEGORY_WEBAPK_API = "android.intent.category.WEBAPK_API";
+    private static final String TAG = "cr_WebApk";
+
+    private static WebApkServiceClient sInstance;
+
+    /** Manages connections between the browser application and WebAPK services. */
+    private WebApkServiceConnectionManager mConnectionManager;
+
+    public static WebApkServiceClient getInstance() {
+        if (sInstance == null) {
+            sInstance = new WebApkServiceClient();
+        }
+        return sInstance;
+    }
+
+    private WebApkServiceClient() {
+        mConnectionManager = new WebApkServiceConnectionManager(CATEGORY_WEBAPK_API, null);
+    }
+
     /**
-     * Connect to a WebAPK's bound service, build a notification and hand it over to the WebAPK to
+     * Connects to a WebAPK's bound service, builds a notification and hand it over to the WebAPK to
      * display. Handing over the notification makes the notification look like it originated from
      * the WebAPK - not Chrome - in the Android UI.
      */

[pkotwicz, 2017/07/19 16:09:13]

Can you make this a non-static function?

Nit: "hand it over" -> "hands it over"

[Xi Han, 2017/07/21 20:36:33]

Done.

@@ -68,13 +88,11 @@ public class WebApkNotificationClient {
             }
         };
 
-        WebApkServiceConnectionManager.getInstance().connect(
+        getInstance().mConnectionManager.connect(
                 ContextUtils.getApplicationContext(), webApkPackage, connectionCallback);
     }
 
-    /**
-     * Cancel notification previously shown by WebAPK.
-     */
+    /** Cancels notification previously shown by WebAPK. */
     public static void cancelNotification(
             String webApkPackage, final String platformTag, final int platformID) {

[pkotwicz, 2017/07/19 16:09:13]

Can you make this a non-static function?

[Xi Han, 2017/07/21 20:36:34]

Done.

         final ApiUseCallback connectionCallback = new ApiUseCallback() {
@@ -83,10 +101,18 @@ public class WebApkNotificationClient {
                 api.cancelNotification(platformTag, platformID);
             }
         };
-        WebApkServiceConnectionManager.getInstance().connect(
+
+        getInstance().mConnectionManager.connect(
                 ContextUtils.getApplicationContext(), webApkPackage, connectionCallback);
     }
 
+    /** Disconnects all the connections from WebAPK services. */
+    public static void disconnectWebApkService(Context appContext) {

[pkotwicz, 2017/07/19 16:09:13]

We don't pass the context for notify() so we don't need to pass the context for
disconnectWebApkService() either

Nits:
- Rename this function to disconnectAll(). What you are disconnecting from
should be obvious from the class name.
- "from WebAPK services" -> "to WebAPK services" (because Chrome initiated the
connection to the WebAPKService)

[Xi Han, 2017/07/21 20:36:34]

Done.

+        if (sInstance == null) return;
+
+        sInstance.mConnectionManager.disconnect(appContext);
+    }
+
     /** Decodes bitmap from WebAPK's resources. */
     private static Bitmap decodeImageResource(String webApkPackage, int resourceId) {
         PackageManager packageManager = ContextUtils.getApplicationContext().getPackageManager();