Cronet: a framework for providing alternative Cronet implementations

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

+// 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.support.annotation.VisibleForTesting;
+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.
+ * <p/>
+ * <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#CronetEngine.Builder(android.content.Context)}.

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

Might want to put this NOTE at the top.

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

See the answer to the similar comment.

+ */
+public class CronetProviders {
+    private static final String TAG = CronetProvider.class.getSimpleName();
+
+    /**
+     * Name of the platform Cronet provider.

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

I don't understand this.  I don't know what "platform" refers to.  I think this
should be moved to CronetProvider.  How about:
/**
 * String returned by {@link CronetProvider#getName} for {@link CronetProvider}
that provides
 * Cronet implementation based on the system's {@link
java.net.HttpURLConnection} implementation.
 * This implementation offers significantly degraded performance relative to
native Cronet
 * implementations (see {@link #NATIVE_PROVIDER_NAME}).
 */
public static final String LOW_PERFORMANCE_PROVIDER_NAME = "Low-Performance";

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

Nice. Changed the docs. Regarding the naming, if the providers are named based
on performance it is not clear how to name other providers.

[pauljensen, 2017/01/23 14:44:28]

You didn't address my comment about "platform".  I still don't understand what
"platform" refers to.  I still think we need a new name and value for this
variable.  We really don't want embedders using this by accident, they'll get
performance worse than Android's HttpURLConnection, and think Cronet is a waste.

You didn't address my request to move this to CronetProvider.  I think it makes
more sense there as it can be beside the getName() function where it is returned
from.

[kapishnikov, 2017/01/23 18:16:52]

Renamed to PROVIDER_NAME_FALLBACK & PROVIDER_NAME_APP_PACKAGED and moved them to
CronetProvider.

+     */
+    public static final String PROVIDER_NAME_PLATFORM = "Platform-Cronet-Provider";
+
+    /**
+     * Name of the native platform provider.

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

nit: s/platform/Cronet/ or remove.

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

Done.

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

I don't understand this.  I don't know what "platform" refers to.  I think this
should be moved to CronetProvider.  I think we need to differentiate this from
other native providers.  How about:
/**
 * String returned by {@link CronetProvider#getName} for {@link CronetProvider}
that provides
 * high performance Cronet native implementation distributed with the
application.
 */
public static final String APP_PACKAGED_NATIVE_PROVIDER_NAME =
"App-Packaged-Native";

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

Changed the docs similar to PROVIDER_NAME_PLATFORM. Needs more discussion.

+     */
+    public static final String PROVIDER_NAME_NATIVE = "Native-Cronet-Provider";
+
+    /**
+     * 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 sInstance = new CronetProviders();
+
+    @VisibleForTesting
+    CronetProviders() {}
+
+    public static CronetProviders getInstance() {

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

needs javadoc, or preferably just remove if we instead just make
getAllProviders() a static member of CronetProvider

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

If we make it static, it will be difficult to mock it and difficult to test
classes that use it. We should see if there is a workaround. Added javadoc.

+        return sInstance;
+    }
+
+    /**
+     * 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 providers 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) {

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

How about moving this to CronetProvider (after making it static like Misha
recommended)?  I think that would simplify things significantly

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

"Available" is confusing because we also use the term "Enabled".  How about just
getAllProviders?

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

Done.

+        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;
+    }
+
+    /**
+     * Adds a new provider referenced by the class name to the end of the list.
+     *
+     * @param className the class name of the provider that should be instantiated.
+     * @param providers the list of providers to add the new provider to.
+     * @return {@code true} if the provider was added to the list; {@code false}
+     *         if the provider couldn't be instantiated.
+     */
+    private static boolean addCronetProviderImplByClassName(
+            Context context, String className, List<CronetProvider> providers, boolean logError) {
+        ClassLoader loader = CronetProviders.class.getClassLoader();

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

perhaps this should use context.getClassLoader()?

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

This is a good point. I am not sure in what circumstances it can be different
from the one that loaded this class.

+        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 static void logReflectiveOperationException(
+            String className, boolean logError, Exception e) {
+        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 {@code true} if the provider was added to the list; {@code false}
+     *         if the provider wasn't listed, couldn't be found or instantiated.
+     * @

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

spurious @

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

Forgot to add @throws RuntimeException. Done.

+     */
+    private static boolean addCronetProviderFromResourceFile(
+            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) {

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

now that added is used once you can combine these lines into "if (!addCronet..."

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

Done.

+            throw new RuntimeException("Unable to instantiate Cronet implementation class "
+                    + className + " that is listed as in the app string resource file under"
+                    + RES_KEY_CRONET_IMPL_CLASS + " key");
+        }
+        return true;
+    }
+}