Cronet: a framework for providing alternative Cronet implementations

components/cronet/android/api/src/org/chromium/net/CronetProviders.java

Index: components/cronet/android/api/src/org/chromium/net/CronetProviders.java
diff --git a/components/cronet/android/api/src/org/chromium/net/CronetProviders.java b/components/cronet/android/api/src/org/chromium/net/CronetProviders.java
new file mode 100644
index 0000000000000000000000000000000000000000..5caef5b26d044339d2baceeee54f3239c919ed50
--- /dev/null
+++ b/components/cronet/android/api/src/org/chromium/net/CronetProviders.java
@@ -0,0 +1,138 @@
+// 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.net;
+
+import android.content.Context;
+import android.util.Log;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.InvocationTargetException;
+import java.util.ArrayList;
+import java.util.List;
+
+/**
+ * Provides the list of available {@link CronetProvider} instances.
+ */

[pauljensen, 2017/01/18 17:03:04]

what about prefacing this with something like:

<b>NOTE:</b> This class is for advanced users that want to select a particular
Cronet implementation.  Most users should simply use {@code new} {@link
CronetEngine.Builder#Builder}.
<p>

[kapishnikov, 2017/01/19 01:25:24]

Done. After the description; otherwise, it will not look good in JavaDocs.

+public class CronetProviders {
+    private static final String TAG = CronetProvider.class.getSimpleName();
+
+    /**
+     * The key in the app string resource file that is be searched
+     * for an alternative implementation.
+     */
+    private static final String RES_KEY_CRONET_IMPL_CLASS = "CronetProviderClassName";
+
+    private static CronetProviders sSingleton = new CronetProviders();

[pauljensen, 2017/01/18 17:03:04]

nit: rename to sInstance

[kapishnikov, 2017/01/19 01:25:24]

Done.

+
+    private CronetProviders() {}
+
+    public static CronetProviders instance() {

[pauljensen, 2017/01/18 17:03:04]

I think this kind of function is normally called getInstance

[kapishnikov, 2017/01/19 01:25:24]

Done.

+        return sSingleton;
+    }
+
+    /**
+     * Name of the Java {@link CronetProvider} class.
+     */
+    private static final String JAVA_CRONET_PROVIDER_CLASS =
+            "org.chromium.net.impl.JavaCronetProvider";
+
+    /**
+     * Name of the native {@link CronetProvider} class.
+     */
+    private static final String NATIVE_CRONET_PROVIDER_CLASS =
+            "org.chromium.net.impl.NativeCronetProvider";
+
+    /**
+     * Returns the found list of available providers.
+     * The providers are added in the following order:
+     * <ul>
+     * <li>Class name specified as "CronetProviderClassName" application string resource.</li>
+     * <li>The default Cronet implementation</li>
+     * </ul>
+     *
+     * Some of the returned provider may be in a disabled state and should be enabled by
+     * the invoker. See {@link CronetProvider#isEnabled()}.
+     *
+     * @return the list of available providers.
+     */
+    public List<CronetProvider> getAvailableProviders(Context context) {
+        List<CronetProvider> providers = new ArrayList<>();
+        addCronetProviderFromResourceFile(context, providers);
+        addCronetProviderImplByClassName(context, NATIVE_CRONET_PROVIDER_CLASS, providers, false);
+        addCronetProviderImplByClassName(context, JAVA_CRONET_PROVIDER_CLASS, providers, false);
+        return providers;

[pauljensen, 2017/01/18 17:03:04]

how slow is this?  should we cache the list?

[mef, 2017/01/18 20:57:54]

Can it change? For example if ProviderFromResourceFile is updated to match api
version?

[kapishnikov, 2017/01/19 01:25:24]

It should not be slow and normally it will be called only once. I think it is
better to create new instances of the providers since they may have some logic
in the constructor that depends on the current environment state. It can
potentially have other negative implications. E.g. if a provider is not
thread-safe and two different threads obtain the same provider instance, there
can be threading issues.

[mef, 2017/01/19 22:57:46]

In this case what's the point of mInstance and getInstance()? It seems like this
method could also be static.

[kapishnikov, 2017/01/20 16:21:53]

A singleton provides more flexibility since it can act as a normal class. E.g.,
it can be mocked and extended. That is how the CronetEngineBuilderTest extends
CronetProviders and injects it into
CronetEngine.Builder.getEnabledCronetProviders(getContext(). That would be
difficult if the class contained only static methods and fields.

[pauljensen, 2017/01/20 17:13:34]

Can you quantify that?
Won't it be called every time we want to create a CronetEngine?

[kapishnikov, 2017/01/20 21:48:37]

Yes, it will be called every time the default CronetEngine.Bulder is created. It
will not be called multiple times if the same builder is used to create multiple
engines. But I agree, it is better to cache the providers and specify that they
should be thread-safe & that they should not rely on the environment state that
was in place when the provider constructor was called.

+    }
+
+    /**
+     * Adds a new provider referenced by the class name to the end of the list.
+     *
+     * @param className the class name of the provider that shoul dbe instantiated.

[mef, 2017/01/18 20:57:54]

nit: s/shoul dbe/should be/

[kapishnikov, 2017/01/19 01:25:24]

Done.

+     * @param providers the list of providers to add the new provider to.
+     * @return true if the provider was added.
+     */
+    private boolean addCronetProviderImplByClassName(

[pauljensen, 2017/01/18 17:03:04]

nit: prefix with "maybe" as it can silently fail.

[kapishnikov, 2017/01/19 01:25:24]

The return value implies that the provider may not be added in some
circumstances. I improved the description.

[pauljensen, 2017/01/20 17:13:34]

Change the javadoc to start with "Attempts to add"

[kapishnikov, 2017/01/20 21:48:38]

Done.

+            Context context, String className, List<CronetProvider> providers, boolean logError) {
+        ClassLoader loader = this.getClass().getClassLoader();
+        try {
+            Class<? extends CronetProvider> providerClass =
+                    loader.loadClass(className).asSubclass(CronetProvider.class);
+            Constructor<? extends CronetProvider> ctor =
+                    providerClass.getConstructor(Context.class);
+            providers.add(ctor.newInstance(context));
+            return true;
+        } catch (InstantiationException e) {
+            logReflectiveOperationException(className, logError, e);
+        } catch (InvocationTargetException e) {
+            logReflectiveOperationException(className, logError, e);
+        } catch (NoSuchMethodException e) {
+            logReflectiveOperationException(className, logError, e);
+        } catch (IllegalAccessException e) {
+            logReflectiveOperationException(className, logError, e);
+        } catch (ClassNotFoundException e) {
+            logReflectiveOperationException(className, logError, e);
+        }
+        return false;
+    }
+
+    /**
+     * De-duplicates exception handling logic in {@link #addCronetProviderImplByClassName}.
+     * It should be removed when support of API Levels lower than 19 is deprecated.
+     */
+    private void logReflectiveOperationException(String className, boolean logError, Exception e) {

[pauljensen, 2017/01/18 17:03:04]

nit: for this and all other private functions, can they be static?

[kapishnikov, 2017/01/19 01:25:24]

Done.

+        if (logError) {
+            Log.e(TAG, "Unable to load provider class: " + className, e);
+        } else {
+            Log.d(TAG, "Tried to load " + className + " provider class but it wasn't"
+                            + " included in the app classpath");
+        }
+    }
+
+    /**
+     * Adds a provider specified in the app resource file to the end of the provider list.
+     *
+     * @param providers the list of providers to add the new provider to.
+     * @return true if the provider was added.
+     */
+    private boolean addCronetProviderFromResourceFile(

[pauljensen, 2017/01/18 17:03:04]

nit: prefix with "maybe" as it can silently fail.

[kapishnikov, 2017/01/19 01:25:24]

Added extra documentation to the @return.

[pauljensen, 2017/01/20 17:13:34]

Change the javadoc to start with "Attempts to add"

[kapishnikov, 2017/01/20 21:48:37]

Done. Also added @throws RuntimeException

+            Context context, List<CronetProvider> providers) {
+        int resId = context.getResources().getIdentifier(
+                RES_KEY_CRONET_IMPL_CLASS, "string", context.getPackageName());
+        // Resource not found
+        if (resId == 0) {
+            // The resource wasn't included in the app; therefore, there is nothing to add.
+            return false;
+        }
+        String className = context.getResources().getString(resId);
+
+        boolean added = addCronetProviderImplByClassName(context, className, providers, true);
+
+        if (!added) {
+            Log.e(TAG, "Unable to instantiate Cronet implementation class " + className
+                            + " that is listed as in the app string resource file under"
+                            + RES_KEY_CRONET_IMPL_CLASS + " key");

[pauljensen, 2017/01/18 17:03:04]

I think this should throw, not just log.  I think this represents a serious
misconfiguration.

[mef, 2017/01/18 20:57:54]

I disagree, this could represent that provider isn't available on the device.

[kapishnikov, 2017/01/19 01:25:24]

Actually, the provider should be packaged inside the app apk, so we can throw an
exception since it is wrong to reference a provider that cannot be found.
Changed.

+        }
+        return added;
+    }
+}