Refactor WebApkServiceConnectionManager.

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

Index: chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java
diff --git a/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java
new file mode 100644
index 0000000000000000000000000000000000000000..6e7de61b3e4fb280ce18196c2e9bbaa03962f538
--- /dev/null
+++ b/chrome/android/webapk/libs/client/src/org/chromium/webapk/lib/client/WebApkIdentityServiceClient.java
@@ -0,0 +1,116 @@
+// 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.Context;
+import android.content.pm.ApplicationInfo;
+import android.content.pm.PackageManager;
+import android.os.Bundle;
+import android.os.IBinder;
+import android.os.RemoteException;
+import android.text.TextUtils;
+import android.util.Log;
+
+import org.chromium.webapk.lib.common.IdentityService.IIdentityService;
+import org.chromium.webapk.lib.common.WebApkMetaDataKeys;
+
+/**
+ * Manages static global connections between the Chrome application and "WebAPK Identity Service".
+ * Each WebAPK has its own "WebAPK Identity Service".
+ */
+public class WebApkIdentityServiceClient extends WebApkServiceConnectionManager {
+    /** Used to notified the consumer after checking the WebAPK's runtime host asynchronously. */
+    public interface CheckRuntimeHostCallback { void onCheckedRuntimeHost(boolean isValid); }
+
+    private static final String CATEGORY_WEBAPK_IDENTITY_SERVICE =
+            "org.webapk.IDENTITY_SERVICE_API";
+    private static final String TAG = "cr_WebApkIdentityService";
+    private static final int SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST = 6;

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

Shouldn't this be 15?

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

Shell apk number between 6 and 15 is: we introduce the "choose-host-browser"
logic but don't have identity service. If a shell apk version is in this range,
we don't have a way to know exactly which browser is the runtime host. Before
version 6, we probably only have browser-WebAPKs installed. If the browser who
is querying the runtime host matches the specified runtime host, we think it is
the host browser. Otherwise it is not.

[pkotwicz, 2017/07/18 17:30:46]

Good point.

I see what you are saying. Please add comments to
maybeExtractRuntimeHostFromMetaData() to explain the logic. We should also have
JUnit tests for this logic to ensure that the meta data is ignored when it
should be

[Xi Han, 2017/07/18 22:36:51]

I add a link to here in the comments of maybeExtractRuntimeHostFromMetaData().

+
+    private static WebApkIdentityServiceClient sInstance;
+
+    public static WebApkIdentityServiceClient getInstance() {
+        if (sInstance == null) {
+            sInstance = new WebApkIdentityServiceClient();
+        }
+        return sInstance;
+    }
+
+    private WebApkIdentityServiceClient() {
+        super(CATEGORY_WEBAPK_IDENTITY_SERVICE);
+    }
+
+    /**
+     * Checks the runtime host of a WebAPK and notifies the caller. The runtime host could be null
+     * if the WebAPK either doesn't have an Identity Service or hasn't bind to any browser yet.
+     * @param context The app context.
+     * @param webApkPackageName The package name of the WebAPK.
+     * @param callback The callback to be called after querying the runtime host is done.
+     */
+    public static void checkWebApkRuntimeHostAsync(final Context context,
+            final String webApkPackageName, final CheckRuntimeHostCallback callback) {
+        final String packageName = context.getPackageName();
+        WebApkServiceConnectionManager.ConnectionCallback connectionCallback =
+                new ConnectionCallback() {
+                    @Override
+                    public void onConnected(IBinder service) {
+                        if (service == null) {
+                            WebApkIdentityServiceClient.notify(packageName, callback,
+                                    maybeExtractRuntimeHostFromMetaData(
+                                            context, webApkPackageName));
+                            return;
+                        }
+
+                        IIdentityService identityService =
+                                IIdentityService.Stub.asInterface(service);
+                        String runtimeHost = null;
+                        try {
+                            runtimeHost = identityService.getRuntimeHostBrowserPackageName();
+                        } catch (RemoteException e) {
+                            Log.w(TAG, "Failed to get runtime host from the Identity Service.");
+                        } finally {

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

Can you move this outside of the finally{} block?

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

Done.

+                            WebApkIdentityServiceClient.notify(packageName, callback, runtimeHost);
+                        }
+                    }
+                };
+        getInstance().connect(context, webApkPackageName, connectionCallback);
+    }
+
+    /** Notifies the caller whether the runtime host matches the current app. */
+    public static void notify(

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

Nits:
1) Can you rename this function to onGotWebApkHostBrowser()
The function comment could be: "Called after fetching the WebAPK's host
browser."
It is weird for the function to be called notify() because the function
processes its parameters
2) Can you make |callback| the last parameter?
I think that making output parameters the last parameter is a convention (I have
not looked it up in the style guide though)
3) Can you rename runtimeHost to webApkRuntimeHostPackageName
This makes it clearer:
- what the runtime host is (it is a property of the WebAPK)
- that both parameters are package names
4) Can you add @param comments for the function parameters?

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

Done.

+            String appPackageName, CheckRuntimeHostCallback callback, String runtimeHost) {
+        callback.onCheckedRuntimeHost(
+                !TextUtils.isEmpty(runtimeHost) && runtimeHost.contentEquals(appPackageName));
+    }
+
+    /**
+     * Only returns the runtime host if
+     * - the runtime host specified in the AndroidManifest.xml matches the current app name.

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

Can you please fix the comment?

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

Done.

+     * AND
+     * - The shell Apk version is less than SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST.

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

Nit: Apk -> APK

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

Done.

+     */
+    private static String maybeExtractRuntimeHostFromMetaData(
+            Context context, String webApkPackageName) {
+        Bundle metadata = readMetaData(context, webApkPackageName);
+        if (metadata.getInt(WebApkMetaDataKeys.SHELL_APK_VERSION)
+                >= SHELL_APK_VERSION_SUPPORTING_SWITCH_RUNTIME_HOST) {
+            return null;
+        }
+
+        return metadata.getString(WebApkMetaDataKeys.RUNTIME_HOST);
+    }
+
+    /** Returns the <meta-data> in the Android Manifest of the given package name. */
+    private static Bundle readMetaData(Context context, String packageName) {
+        ApplicationInfo ai = null;
+        try {
+            ai = context.getPackageManager().getApplicationInfo(
+                    packageName, PackageManager.GET_META_DATA);
+        } catch (PackageManager.NameNotFoundException e) {
+            return null;
+        }
+        return ai.metaData;
+    }
+}