Cronet API Refactoring

components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java

Index: components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
diff --git a/components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java b/components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
new file mode 100644
index 0000000000000000000000000000000000000000..1fa5d86cf95110d00b9071bded9bac34bd6b1264
--- /dev/null
+++ b/components/cronet/android/java/src/org/chromium/net/impl/CronetEngineBase.java
@@ -0,0 +1,127 @@
+// Copyright 2016 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.impl;
+
+import static org.chromium.net.UrlRequest.Builder.REQUEST_PRIORITY_HIGHEST;
+import static org.chromium.net.UrlRequest.Builder.REQUEST_PRIORITY_IDLE;
+import static org.chromium.net.UrlRequest.Builder.REQUEST_PRIORITY_LOW;
+import static org.chromium.net.UrlRequest.Builder.REQUEST_PRIORITY_LOWEST;
+import static org.chromium.net.UrlRequest.Builder.REQUEST_PRIORITY_MEDIUM;
+
+import android.support.annotation.IntDef;
+
+import org.chromium.net.BidirectionalStream;
+import org.chromium.net.CronetEngine;
+import org.chromium.net.ExperimentalCronetEngine;
+import org.chromium.net.UrlRequest;
+
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.reflect.Constructor;
+import java.net.URL;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.Executor;
+
+/**
+ * Base class of {@link CronetUrlRequestContext} and {@link JavaCronetEngine} that contains
+ * shared logic.
+ */
+public abstract class CronetEngineBase extends ExperimentalCronetEngine {
+    private static final String CRONET_URL_REQUEST_CONTEXT =
+            "org.chromium.net.impl.CronetUrlRequestContext";
+
+    /**
+     * Creates a {@link UrlRequest} object. All callbacks will
+     * be called on {@code executor}'s thread. {@code executor} must not run
+     * tasks on the current thread to prevent blocking networking operations
+     * and causing exceptions during shutdown.
+     *
+     * @param url {@link URL} for the request.
+     * @param callback callback object that gets invoked on different events.
+     * @param executor {@link Executor} on which all callbacks will be invoked.
+     * @param priority priority of the request which should be one of the
+     *         {@link UrlRequest.Builder#REQUEST_PRIORITY_IDLE REQUEST_PRIORITY_*}
+     *         values.
+     * @param requestAnnotations Objects to pass on to
+     *        {@link org.chromium.net.RequestFinishedInfo.Listener}.
+     * @param disableCache disables cache for the request.
+     *         If context is not set up to use cache this param has no effect.
+     * @param disableConnectionMigration disables connection migration for this
+     *         request if it is enabled for the session.
+     * @param allowDirectExecutor whether executors used by this request are permitted
+     *         to execute submitted tasks inline.
+     * @return new request.
+     */
+    protected abstract UrlRequestBase createRequest(String url, UrlRequest.Callback callback,
+            Executor executor, int priority, Collection<Object> requestAnnotations,
+            boolean disableCache, boolean disableConnectionMigration, boolean allowDirectExecutor);

[pauljensen, 2016/09/26 14:51:22]

I'm not a fan of these odd functions that try and take every builder option as
an argument.  Seems ripe for an accidental argument order swap.  Also, the
JavaUrlRequest might not need many of these options.  I feel like we should
remove all these options and just offer getters on the Builders.  Perhaps not
for this CL.

[kapishnikov, 2016/09/27 18:38:26]

Acknowledged. I agree.

+
+    /**
+     * Creates a {@link BidirectionalStream} object. {@code callback} methods will
+     * be invoked on {@code executor}. {@code executor} must not run
+     * tasks on the current thread to prevent blocking networking operations
+     * and causing exceptions during shutdown.
+     *
+     * @param url the URL for the stream
+     * @param callback the object whose methods get invoked upon different events
+     * @param executor the {@link Executor} on which all callbacks will be called
+     * @param httpMethod the HTTP method to use for the stream
+     * @param requestHeaders the list of request headers
+     * @param priority priority of the stream which should be one of the
+     *         {@link BidirectionalStream.Builder#STREAM_PRIORITY_IDLE STREAM_PRIORITY_*}
+     *         values.
+     * @param delayRequestHeadersUntilFirstFlush whether to delay sending request
+     *         headers until flush() is called, and try to combine them
+     *         with the next data frame.
+     * @param requestAnnotations Objects to pass on to {@link RequestFinishedInfo.Listener}.
+     * @return a new stream.
+     */
+    public abstract BidirectionalStream createBidirectionalStream(String url,
+            BidirectionalStream.Callback callback, Executor executor, String httpMethod,
+            List<Map.Entry<String, String>> requestHeaders,
+            @BidirectionalStreamBuilderImpl.StreamPriority int priority,
+            boolean delayRequestHeadersUntilFirstFlush, Collection<Object> requestAnnotations);
+
+    /**
+     * @return {@code true} if the engine is enabled.
+     */
+    public abstract boolean isEnabled();
+
+    static CronetEngine createCronetEngine(CronetEngineBuilderImpl builder) {
+        CronetEngine cronetEngine = null;
+        try {
+            Class<? extends CronetEngineBase> engineClass =
+                    builder.getContext()
+                            .getClassLoader()
+                            .loadClass(CRONET_URL_REQUEST_CONTEXT)
+                            .asSubclass(CronetEngineBase.class);
+            Constructor<? extends CronetEngineBase> constructor =
+                    engineClass.getConstructor(CronetEngineBuilderImpl.class);
+            CronetEngineBase possibleEngine = constructor.newInstance(builder);

[pauljensen, 2016/09/26 14:51:22]

eh what's the point of this?  I don't think we should need any reflection in the
impl.

[kapishnikov, 2016/09/27 18:38:26]

Agree. I will fix it.

[kapishnikov, 2016/09/30 16:01:43]

Done.

+            if (possibleEngine.isEnabled()) {
+                cronetEngine = possibleEngine;
+            }
+        } catch (ClassNotFoundException e) {
+            // Leave as null.
+        } catch (Exception e) {
+            throw new IllegalStateException("Cannot instantiate: " + CRONET_URL_REQUEST_CONTEXT, e);
+        }
+        return cronetEngine;
+    }
+
+    @Override
+    public UrlRequest.Builder newUrlRequestBuilder(
+            String url, UrlRequest.Callback callback, Executor executor) {
+        return new UrlRequestBuilderImpl(url, callback, executor, this);
+    }
+
+    @IntDef({
+            REQUEST_PRIORITY_IDLE, REQUEST_PRIORITY_LOWEST, REQUEST_PRIORITY_LOW,
+            REQUEST_PRIORITY_MEDIUM, REQUEST_PRIORITY_HIGHEST,
+    })
+    @Retention(RetentionPolicy.SOURCE)
+    public @interface RequestPriority {}
+}