WebView: Refactor PlatformServiceBridge.getInstance

android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java

Index: android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
diff --git a/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java b/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
index 0ced33e7389f288c0330013f211c76d31f63f0a4..dc5b348a2d2c4d4ccc2baeb56b09ec00e4a555c6 100644
--- a/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
+++ b/android_webview/java/src/org/chromium/android_webview/PlatformServiceBridge.java
@@ -9,6 +9,7 @@ import android.webkit.ValueCallback;
 
 import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
+import org.chromium.base.VisibleForTesting;
 
 import java.lang.reflect.InvocationTargetException;
 
@@ -21,41 +22,61 @@ public class PlatformServiceBridge {
     private static final String PLATFORM_SERVICE_BRIDGE =
             "com.android.webview.chromium.PlatformServiceBridgeGoogle";
 
-    private static final Object sInstanceLock = new Object();
     private static PlatformServiceBridge sInstance;
 
     protected PlatformServiceBridge() {}
 
-    public static PlatformServiceBridge getInstance(Context appContext) {
-        // TODO(timvolodine): consider removing the lock, crbug.com/681805.
-        synchronized (sInstanceLock) {
-            if (sInstance != null) {
-                return sInstance;
-            }
-
-            // Try to get a specialized service bridge.
-            try {
-                Class<?> cls = Class.forName(PLATFORM_SERVICE_BRIDGE);
-                sInstance = (PlatformServiceBridge) cls.getDeclaredConstructor(Context.class)
-                                    .newInstance(appContext);
-                return sInstance;
-            } catch (ClassNotFoundException e) {
-                // This is not an error; it just means this device doesn't have specialized
-                // services.
-            } catch (IllegalAccessException | IllegalArgumentException | InstantiationException
-                    | NoSuchMethodException e) {
-                Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + ": " + e);
-            } catch (InvocationTargetException e) {
-                Log.e(TAG, "Failed invocation to get " + PLATFORM_SERVICE_BRIDGE + ":",
-                        e.getCause());
-            }
-
-            // Otherwise, get the generic service bridge.
-            sInstance = new PlatformServiceBridge();
+    // Should be called at most once.
+    public static PlatformServiceBridge createInstance(Context appContext) {

[Tobias Sargeant, 2017/04/03 10:40:06]

Given that this only ever returns sInstance (or throws), maybe it shouldn't
return a value?

[paulmiller, 2017/04/03 23:34:16]

Why is that? If people are going to be calling getInstance immediately after
createInstance, it seems more convenient to combine them.

[Tobias Sargeant, 2017/04/04 12:30:47]

My personal preference if there's a createInstance() and a getInstance() is not
to favour convenience if that means that sometimes you'll call createInstance
without using its return value. getOrCreateInstance() makes that question
irrelevant however.

+        // Just to avoid race conditions - nothing special about the UI thread.
+        ThreadUtils.assertOnUiThread();
+
+        if (sInstance != null) {
+            throw new IllegalStateException("PlatformServiceBridge already created");
+        }
+
+        // Try to get a specialized service bridge.
+        try {
+            Class<?> cls = Class.forName(PLATFORM_SERVICE_BRIDGE);
+            sInstance = (PlatformServiceBridge) cls.getDeclaredConstructor(Context.class)
+                                .newInstance(appContext);
+            return sInstance;
+        } catch (ClassNotFoundException e) {
+            // This is not an error; it just means this device doesn't have specialized
+            // services.
+        } catch (IllegalAccessException | IllegalArgumentException | InstantiationException
+                | NoSuchMethodException e) {
+            Log.e(TAG, "Failed to get " + PLATFORM_SERVICE_BRIDGE + ": " + e);
+        } catch (InvocationTargetException e) {
+            Log.e(TAG, "Failed invocation to get " + PLATFORM_SERVICE_BRIDGE + ":", e.getCause());
         }
+
+        // Otherwise, get the generic service bridge.
+        sInstance = new PlatformServiceBridge();
+
         return sInstance;
     }
 
+    // May be called many times, by many threads.
+    public static PlatformServiceBridge getInstance() {
+        if (sInstance == null) {
+            throw new IllegalStateException("PlatformServiceBridge not created");
+        }
+
+        return sInstance;
+    }
+
+    // Provide a mocked PlatformServiceBridge for testing.
+    @VisibleForTesting
+    public static void injectInstance(PlatformServiceBridge testBridge) {
+        sInstance = testBridge;
+    }
+
+    // TODO(paulmiller): Remove after changing downstream users.
+    public static PlatformServiceBridge getInstance(Context appContext) {
+        return getInstance();
+    }
+
     // Can WebView use Google Play Services (a.k.a. GMS)?
     public boolean canUseGms() {
         return false;