Prepare to call GMS APIs from WebView

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

 // Copyright 2015 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.android_webview;
 
 import android.content.Context;
 import android.webkit.ValueCallback;
 
 import org.chromium.base.Log;
 
 import java.lang.reflect.InvocationTargetException;
 
 /**
  * This class manages platform-specific services. (i.e. Google Services) The platform
  * should extend this class and use this base class to fetch their specialized version.
  */
 public class PlatformServiceBridge {
     private static final String TAG = "PlatformServiceBrid-";
     private static final String PLATFORM_SERVICE_BRIDGE =
             "com.android.webview.chromium.PlatformServiceBridgeGoogle";
 
     private static PlatformServiceBridge sInstance;
 
     protected PlatformServiceBridge() {}
 
-    public static PlatformServiceBridge getInstance(Context applicationContext) {
+    public static PlatformServiceBridge getInstance(Context appContext) {
         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(applicationContext);
+                    .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;
     }
 
     // Try to enable WebView to use Google Play Services (a.k.a. GMS) APIs. Return true on success.
     // Do not use GMS APIs before this has returned true, or if it returns false. This can be called
     // from multiple threads, so long as no thread uses GMS APIs before at least one call has
     // returned true. (The easy way is for each thread to wait for its own call to return true.)
     public boolean tryEnableGms() {

[sgurun-gerrit only, 2017/01/04 00:31:53]

remove this method.

         return false;
     }
 
-    public void setMetricsSettingListener(ValueCallback<Boolean> callback) {}
+    // Overriding implementations may call "callback" asynchronously, but they must still call

[gsennton, 2017/01/04 10:09:55]

So we will use this method by spawning a new thread, calling queryMetricsEnabled
on that thread, and then wait for that thread to receive the callback? 

[paulmiller, 2017/01/04 18:12:41]

You could do that. I don't. See the AwMetricsServiceClient constructor. It's all
on the main thread; it calls queryMetricsEnabled, and then the thread is free to
do other things, and it'll come back and actually enable metrics whenever the
callback fires.

I could make queryMetricsEnabled take a Handler, so you could specify which
thread you want the callback on. But I didn't imagine anyone would need that. So
I just promised that the callback comes back on the same thread, since that
seemed reasonable and unsurprising. Do you need something else?

[gsennton, 2017/01/04 18:35:04]

Well, I think we (silent feedback) would want to wait until we got a response
from GmsCore before we start uploading minidumps.

Our upload service is triggered on the main thread and then performs minidump
uploading on a worker thread. So I guess we could just ask GmsCore on the main
thread, and later wait for the response inside the worker thread (sleeping until
the main thread receives the value).

[paulmiller, 2017/01/04 18:45:00]

If the worker thread is persistent, yes, that would work. If not, then you don't
need to sleep anywhere. Just spawn the worker in the callback, on the main
thread. Adding the Handler argument is easy, too. Would you use it if I added
it? Then you could query on the main thread, and the callback could run on the
worker thread (which would need to be persistent, and have a Looper).

[gsennton, 2017/01/04 20:31:01]

Oops, I meant waiting in the worker thread as in sleeping until the main thread
received an answer :). It would be a bit fiddly to do as you suggest here I
think (but I might be wrong at this time of day ;)) - because we would need a
special case for when we have explicitly added a command-line-flag to enable the
feature even if the U&D toggle is OFF.

So to clarify, I suggest keeping the current implementation of receiving the
callback on the main thread, have silent-feedback use this by calling gms core
from the main thread, receiving the answer on the main thread, and have the
worker thread that uploads minidumps just sleep until the main thread receives
an answer. Does that sound reasonable/correct? I think this solution is easier
in terms of me not having to change any instrumentation tests ;).

[sgurun-gerrit only, 2017/01/04 20:33:05]

As far as I remember GMSCore always calls you back on the main thread (please
confirm it), which means the callback here is going to be called from main
thread.

I think it is simple to simply require this method to be called from main
thread.

[paulmiller, 2017/01/04 22:26:12]

By default, GMS calls back on the app's UI thread. But you can pass a Handler
for any other thread. We need to pass a Handler anyway, since WebView's UI
thread might not be the app's UI thread.

But, sure. I'll just put ThreadUtils.assertOnUiThread() everywhere.

+    // "callback" on the same thread on which "queryMetricsEnabled" was called.
+    public void queryMetricsEnabled(ValueCallback<Boolean> callback) {

[sgurun-gerrit only, 2017/01/04 00:05:24]

I prefer allowMetricsService() as the method name. 

[paulmiller, 2017/01/04 00:10:15]

Why is that? That sounds like we're setting something rather than getting it. I
also try to avoid words like "service"--I think they're too vague.

[sgurun-gerrit only, 2017/01/04 00:31:53]

queryMetricsValue could be better choice. queryMetricsEnabled does not sound
right to my ear. 

[paulmiller, 2017/01/04 00:54:40]

How about "queryMetricsSetting"? That's slightly more specific than "value".

+        callback.onReceiveValue(false);
+    }
 }