Cronet: a framework for providing alternative Cronet implementations

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

Index: components/cronet/android/api/src/org/chromium/net/CronetEngine.java
diff --git a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
index cd7f42558d03a9e3bcae83a468e22f5c47f6e1ef..cb6fe0ad51d4872db1eebf89cb71ac2a23cb61b3 100644
--- a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
+++ b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
@@ -6,12 +6,17 @@ package org.chromium.net;
 
 import android.content.Context;
 import android.net.http.HttpResponseCache;
+import android.support.annotation.VisibleForTesting;
 
 import java.io.IOException;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
+import java.util.Collections;
+import java.util.Comparator;
 import java.util.Date;
+import java.util.Iterator;
+import java.util.List;
 import java.util.Set;
 import java.util.concurrent.Executor;
 
@@ -63,7 +68,20 @@ public abstract class CronetEngine {
          *                the lifetime of {@code context} unnecessarily.
          */
         public Builder(Context context) {
-            mBuilderDelegate = ImplLoader.load(context);
+            this(createBuilderDelegate(context));
+        }
+
+        /**
+         * Constructs {@link Builder} with a given delegate that provides the actual implementation
+         * of the {@code Builder} methods. This constructor is used only by the internal
+         * implementation.
+         *
+         * @param builderDelegate delegate that provides the actual implementation.
+         *
+         * {@hide}
+         */
+        public Builder(ICronetEngineBuilder builderDelegate) {
+            mBuilderDelegate = builderDelegate;
         }
 
         /**
@@ -282,6 +300,131 @@ public abstract class CronetEngine {
         public CronetEngine build() {
             return mBuilderDelegate.build();
         }
+
+        /**
+         * Creates an implementation of {@link ICronetEngineBuilder} by going through
+         * all available {@link CronetProvider CronetProviders} and selecting one
+         * with the highest priority.

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

Please explain "highest priority"

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

Since an enabled provider should never return null. This method implementation
has been changed to consider only the first element of the list. Changed the
javadoc accordingly.

+         *
+         * @param context Android Context to use.
+         * @return the created {@code ICronetEngineBuilder}.
+         */
+        private static ICronetEngineBuilder createBuilderDelegate(Context context) {
+            ICronetEngineBuilder mBuilderDelegate;

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

local variables should not have "m" prefixes, remove this variable.

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

Deleted.

+            List<CronetProvider> providerList =
+                    getEnabledCronetProviders(context, CronetProviders.getInstance());
+
+            // Try to load implementation using available providers. Assuming that

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

nit: Assuming -> Assume

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

Done.

+            // providers in the beginning of the list have higher priority.
+            ICronetEngineBuilder builderDelegate = null;
+            for (CronetProvider provider : providerList) {
+                Builder builder = provider.createBuilder();
+                if (builder != null) {

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

We should use either isEnabled() or createBuilder() returning non-null to signal
an enabled provider, not both.  I vote for isEnabled(), so we can get rid of
this null check and just write:
builderDelegate = provider.createBuilder().mBuilderDelegate;

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

Agree. createBuilder() now throws an exception that should be propagated to the
embedder.

+                    builderDelegate = builder.mBuilderDelegate;
+                    break;
+                }
+            }
+
+            if (builderDelegate == null) {
+                throw new RuntimeException("Unable to load any Cronet implementation."
+                        + " The list of enabled providers: " + providerList);
+            }
+
+            mBuilderDelegate = builderDelegate;
+            return mBuilderDelegate;
+        }
+
+        /**
+         * Returns the list of available and enabled {@link CronetProvider}. The returned list
+         * is sorted based on the provider versions and types.
+         *
+         * @param context Android Context to use.
+         * @return the sorted list of enabled providers.
+         * @throws RuntimeException is the list of providers is empty or all of the providers
+         *                          are disabled.
+         *
+         * {@hide}

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

I don't think package private appears in javadocs, so no {@hide} needed.

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

Removed.

+         */
+        @VisibleForTesting
+        static List<CronetProvider> getEnabledCronetProviders(

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

Should this and below be moved into CronetProviders? 
Then all sorting / enabled / available logic is located in one place?

Per offline discussion I don't feel strongly and can be convinced otherwise.
Paul, WDYT?

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

I have a strong opinion that CronetProviders should only be responsible for
providing the list of all Cronet providers available by the app. It is
responsibility of the client of the class to select the right one.

+                Context context, CronetProviders providers) {
+            // Check that there is at least one available provider.
+            List<CronetProvider> providerList = providers.getAvailableProviders(context);
+            if (providerList.size() == 0) {
+                throw new RuntimeException("Unable to find any Cronet provider."
+                        + " Have you included all necessary jars?");
+            }
+
+            // Exclude disabled providers from the list.
+            for (Iterator<CronetProvider> i = providerList.iterator(); i.hasNext();) {
+                CronetProvider provider = i.next();
+                if (!provider.isEnabled()) {
+                    i.remove();
+                }
+            }
+
+            // Check that there is at least one enabled provider.
+            if (providerList.size() == 0) {
+                throw new RuntimeException("All found Cronet providers are disabled."

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

nit: found -> available

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

Done.

+                        + " A provider should be enabled before it can be used.");
+            }
+
+            // Sort providers based on version and type.
+            Collections.sort(providerList, new Comparator<CronetProvider>() {
+                @Override
+                public int compare(CronetProvider p1, CronetProvider p2) {
+                    // The platform provider should always be at the end of the list.

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

Can PlatformProvider be just added to the end of the list if it is available?

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

We could extract PlatformProvider from the list, sort it and add it to the end
of the list. I don't think it is easier way to do the sorting; unless we change
CronetProviders interface to add some PlatformProvider specific logic.

+                    if (CronetProviders.PROVIDER_NAME_PLATFORM.equals(p1.getName())) {
+                        return 1;
+                    }
+                    if (CronetProviders.PROVIDER_NAME_PLATFORM.equals(p2.getName())) {
+                        return -1;
+                    }
+                    // A provider with higher version should go first.
+                    return -compareVersions(p1.getVersion(), p2.getVersion());

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

Is it ok if compareVersions throws an exception?

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

It is a good question. Currently it will cause the app stop with the exception.
I think that is what we should do since if a provider has an invalid version
something is wrong.

+                }
+            });
+            return providerList;
+        }
+
+        /**
+         * Compares two strings that contain versions. The string should only contain
+         * dot-separated segments that contain an arbitrary number of digits digits [0-9].
+         *
+         * @param s1 the first string.
+         * @param s2 the second string.
+         * @return -1 if s1<s2, +1 if s1>s2 and 0 if s1=s2. If two versions are equal, the
+         *         version with the higher number of segments is considered to be higher.
+         *
+         * @throws IllegalArgumentException if any of the strings contains an illegal
+         * version number.
+         *
+         * {@hide}

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

I don't think package private appears in javadocs, so no {@hide} needed.

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

Removed.

+         */
+        @VisibleForTesting
+        static int compareVersions(String s1, String s2) {
+            if (s1 == null || s2 == null) {
+                throw new IllegalArgumentException("The input values cannot be null");
+            }
+            String[] s1segments = s1.split("\\.");
+            String[] s2segments = s2.split("\\.");
+            int index = 0;
+            while (s1segments.length > index && s2segments.length > index) {

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

how about:
for (int i = 0; i < s1segments.length && i < s2segments.length; i++) {

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

Neat. Done.

+                try {
+                    int s1segment = Integer.parseInt(s1segments[index]);
+                    int s2segment = Integer.parseInt(s2segments[index]);
+                    if (s1segment != s2segment) {
+                        return Integer.signum(s1segment - s2segment);
+                    }
+                    index += 1;
+                } catch (NumberFormatException e) {
+                    throw new IllegalArgumentException("Unable to convert version segments into"
+                                    + " integers: " + s1segments[index] + " & " + s2segments[index],
+                            e);
+                }
+            }
+            return Integer.signum(s1segments.length - s2segments.length);
+        }
     }
 
     /**