Cronet API Refactoring

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

Index: components/cronet/android/api/src/org/chromium/net/UrlRequest.java
diff --git a/components/cronet/android/api/src/org/chromium/net/UrlRequest.java b/components/cronet/android/api/src/org/chromium/net/UrlRequest.java
index 99239c46f882c2217f46d9a4bb8ad10c116cf2a2..85ef22c674af4ee244e8fcdccc45b300b013c7da 100644
--- a/components/cronet/android/api/src/org/chromium/net/UrlRequest.java
+++ b/components/cronet/android/api/src/org/chromium/net/UrlRequest.java
@@ -4,92 +4,23 @@
 
 package org.chromium.net;
 
-import android.support.annotation.IntDef;
-import android.util.Log;
-import android.util.Pair;
-
-import java.lang.annotation.Retention;
-import java.lang.annotation.RetentionPolicy;
 import java.nio.ByteBuffer;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.Collections;
 import java.util.concurrent.Executor;
 
 /**
  * Controls an HTTP request (GET, PUT, POST etc).
- * Created using {@link UrlRequest.Builder}.
- * Note: All methods must be called on the {@link Executor} passed in during creation.
+ * Created by {@link UrlRequest.Builder}, which can be obtained by calling
+ * {@link CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.

[pauljensen, 2016/10/03 15:22:37]

remove "(String, Callback, Executor)" like you did on line 15

[kapishnikov, 2016/10/03 23:49:28]

Done.

+ * Note: All methods must be called on the {@link Executor} passed to
+ * {@link CronetEngine#newUrlRequestBuilder}.
  */
-public interface UrlRequest {
+public abstract class UrlRequest {
     /**
      * Builder for {@link UrlRequest}s. Allows configuring requests before constructing them
-     * with {@link Builder#build}.
+     * with {@link Builder#build}. The builder can be created by calling
+     * {@link CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.

[pauljensen, 2016/10/03 15:22:37]

ditto

[kapishnikov, 2016/10/03 23:49:28]

Done.

      */
-    public static final class Builder {
-        private static final String ACCEPT_ENCODING = "Accept-Encoding";
-        // All fields are temporary storage of UrlRequest configuration to be
-        // copied to built UrlRequests.
-
-        // CronetEngine to execute request.
-        final CronetEngine mCronetEngine;
-        // URL to request.
-        final String mUrl;
-        // Callback to receive progress callbacks.
-        final Callback mCallback;
-        // Executor to invoke callback on.
-        final Executor mExecutor;
-        // HTTP method (e.g. GET, POST etc).
-        String mMethod;
-        // List of request headers, stored as header field name and value pairs.
-        final ArrayList<Pair<String, String>> mRequestHeaders =
-                new ArrayList<Pair<String, String>>();
-        // Disable the cache for just this request.
-        boolean mDisableCache;
-        // Disable connection migration for just this request.
-        boolean mDisableConnectionMigration;
-        // Priority of request. Default is medium.
-        @RequestPriority int mPriority = REQUEST_PRIORITY_MEDIUM;
-        // Request reporting annotations. Avoid extra object creation if no annotations added.
-        Collection<Object> mRequestAnnotations = Collections.emptyList();
-        // If request is an upload, this provides the request body data.
-        UploadDataProvider mUploadDataProvider;
-        // Executor to call upload data provider back on.
-        Executor mUploadDataProviderExecutor;
-        private boolean mAllowDirectExecutor = false;
-
-        /**
-         * Creates a builder for {@link UrlRequest} objects. All callbacks for
-         * generated {@link UrlRequest} objects will be invoked 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 java.net.URL} for the generated requests.
-         * @param callback callback object that gets invoked on different events.
-         * @param executor {@link Executor} on which all callbacks will be invoked.
-         * @param cronetEngine {@link CronetEngine} used to execute this request.
-         */
-        public Builder(
-                String url, Callback callback, Executor executor, CronetEngine cronetEngine) {
-            if (url == null) {
-                throw new NullPointerException("URL is required.");
-            }
-            if (callback == null) {
-                throw new NullPointerException("Callback is required.");
-            }
-            if (executor == null) {
-                throw new NullPointerException("Executor is required.");
-            }
-            if (cronetEngine == null) {
-                throw new NullPointerException("CronetEngine is required.");
-            }
-            mUrl = url;
-            mCallback = callback;
-            mExecutor = executor;
-            mCronetEngine = cronetEngine;
-        }
-
+    public abstract static class Builder {
         /**
          * Sets the HTTP method verb to use for this request.
          *
@@ -99,13 +30,7 @@ public interface UrlRequest {
          * @param method "GET", "HEAD", "DELETE", "POST" or "PUT".
          * @return the builder to facilitate chaining.
          */
-        public Builder setHttpMethod(String method) {
-            if (method == null) {
-                throw new NullPointerException("Method is required.");
-            }
-            mMethod = method;
-            return this;
-        }
+        public abstract Builder setHttpMethod(String method);
 
         /**
          * Adds a request header.
@@ -114,54 +39,14 @@ public interface UrlRequest {
          * @param value header value.
          * @return the builder to facilitate chaining.
          */
-        public Builder addHeader(String header, String value) {
-            if (header == null) {
-                throw new NullPointerException("Invalid header name.");
-            }
-            if (value == null) {
-                throw new NullPointerException("Invalid header value.");
-            }
-            if (ACCEPT_ENCODING.equalsIgnoreCase(header)) {
-                Log.w("cronet",
-                        "It's not necessary to set Accept-Encoding on requests - cronet will do"
-                                + " this automatically for you, and setting it yourself has no "
-                                + "effect. See https://crbug.com/581399 for details.",
-                        new Exception());
-                return this;
-            }
-            mRequestHeaders.add(Pair.create(header, value));
-            return this;
-        }
+        public abstract Builder addHeader(String header, String value);
 
         /**
          * Disables cache for the request. If context is not set up to use cache,
          * this call has no effect.
          * @return the builder to facilitate chaining.
          */
-        public Builder disableCache() {
-            mDisableCache = true;
-            return this;
-        }
-
-        /**
-         * Disables connection migration for the request if enabled for
-         * the session.
-         * @return the builder to facilitate chaining.
-         *
-         * @hide as experimental.
-         */
-        public Builder disableConnectionMigration() {
-            mDisableConnectionMigration = true;
-            return this;
-        }
-
-        /** @hide */
-        @IntDef({
-                REQUEST_PRIORITY_IDLE, REQUEST_PRIORITY_LOWEST, REQUEST_PRIORITY_LOW,
-                REQUEST_PRIORITY_MEDIUM, REQUEST_PRIORITY_HIGHEST,
-        })
-        @Retention(RetentionPolicy.SOURCE)
-        public @interface RequestPriority {}
+        public abstract Builder disableCache();
 
         /**
          * Lowest request priority. Passed to {@link #setPriority}.
@@ -188,17 +73,13 @@ public interface UrlRequest {
         /**
          * Sets priority of the request which should be one of the
          * {@link #REQUEST_PRIORITY_IDLE REQUEST_PRIORITY_*} values.
-         * The request is given {@link #REQUEST_PRIORITY_MEDIUM} priority if {@link
-         * #setPriority} is not called.
+         * The request is given {@link #REQUEST_PRIORITY_MEDIUM} priority by default.

[pauljensen, 2016/10/03 15:22:37]

can we go back to the old wording?  I think it was clearer

[kapishnikov, 2016/10/03 23:49:27]

Done, but replaced the reference to itself by "this method". Same for
BidirectionalStream.setPriority().

          *
          * @param priority priority of the request which should be one of the
          *         {@link #REQUEST_PRIORITY_IDLE REQUEST_PRIORITY_*} values.
          * @return the builder to facilitate chaining.
          */
-        public Builder setPriority(@RequestPriority int priority) {
-            mPriority = priority;
-            return this;
-        }
+        public abstract Builder setPriority(int priority);
 
         /**
          * Sets upload data provider. Switches method to "POST" if not
@@ -211,21 +92,8 @@ public interface UrlRequest {
          *     {@code Executor} the request itself is using.
          * @return the builder to facilitate chaining.
          */
-        public Builder setUploadDataProvider(
-                UploadDataProvider uploadDataProvider, Executor executor) {
-            if (uploadDataProvider == null) {
-                throw new NullPointerException("Invalid UploadDataProvider.");
-            }
-            if (executor == null) {
-                throw new NullPointerException("Invalid UploadDataProvider Executor.");
-            }
-            if (mMethod == null) {
-                mMethod = "POST";
-            }
-            mUploadDataProvider = uploadDataProvider;
-            mUploadDataProviderExecutor = executor;
-            return this;
-        }
+        public abstract Builder setUploadDataProvider(
+                UploadDataProvider uploadDataProvider, Executor executor);
 
         /**
          * Marks that the executors this request will use to notify callbacks (for
@@ -237,32 +105,7 @@ public interface UrlRequest {
          * It should not be used if your callbacks perform disk I/O, acquire locks, or call into
          * other code you don't carefully control and audit.
          */
-        public Builder allowDirectExecutor() {
-            mAllowDirectExecutor = true;
-            return this;
-        }
-
-        /**
-         * Associates the annotation object with this request. May add more than one.
-         * Passed through to a {@link RequestFinishedInfo.Listener},
-         * see {@link RequestFinishedInfo#getAnnotations}.
-         *
-         * @param annotation an object to pass on to the {@link RequestFinishedInfo.Listener} with a
-         * {@link RequestFinishedInfo}.
-         * @return the builder to facilitate chaining.
-         *
-         * @hide as it's a prototype.
-         */
-        public Builder addRequestAnnotation(Object annotation) {
-            if (annotation == null) {
-                throw new NullPointerException("Invalid metrics annotation.");
-            }
-            if (mRequestAnnotations.isEmpty()) {
-                mRequestAnnotations = new ArrayList<Object>();
-            }
-            mRequestAnnotations.add(annotation);
-            return this;
-        }
+        public abstract Builder allowDirectExecutor();
 
         /**
          * Creates a {@link UrlRequest} using configuration within this
@@ -272,21 +115,7 @@ public interface UrlRequest {
          * @return constructed {@link UrlRequest} using configuration within
          *         this {@link Builder}.
          */
-        public UrlRequest build() {
-            final UrlRequest request = mCronetEngine.createRequest(mUrl, mCallback, mExecutor,
-                    mPriority, mRequestAnnotations, mDisableCache, mDisableConnectionMigration,
-                    mAllowDirectExecutor);
-            if (mMethod != null) {
-                request.setHttpMethod(mMethod);
-            }
-            for (Pair<String, String> header : mRequestHeaders) {
-                request.addHeader(header.first, header.second);
-            }
-            if (mUploadDataProvider != null) {
-                request.setUploadDataProvider(mUploadDataProvider, mUploadDataProviderExecutor);
-            }
-            return request;
-        }
+        public abstract UrlRequest build();
     }
 
     /**
@@ -299,7 +128,7 @@ public interface UrlRequest {
      * {@link java.util.concurrent.Executor} used during construction of the
      * {@code UrlRequest}.
      */
-    public abstract class Callback {
+    public abstract static class Callback {
         /**
          * Invoked whenever a redirect is encountered. This will only be invoked
          * between the call to {@link UrlRequest#start} and
@@ -403,16 +232,6 @@ public interface UrlRequest {
      * Request status values returned by {@link #getStatus}.
      */
     public static class Status {
-        /** @hide */
-        @IntDef({
-                INVALID, IDLE, WAITING_FOR_STALLED_SOCKET_POOL, WAITING_FOR_AVAILABLE_SOCKET,
-                WAITING_FOR_DELEGATE, WAITING_FOR_CACHE, DOWNLOADING_PROXY_SCRIPT,
-                RESOLVING_PROXY_FOR_URL, RESOLVING_HOST_IN_PROXY_SCRIPT, ESTABLISHING_PROXY_TUNNEL,
-                RESOLVING_HOST, CONNECTING, SSL_HANDSHAKE, SENDING_REQUEST, WAITING_FOR_RESPONSE,
-                READING_RESPONSE,
-        })
-        @Retention(RetentionPolicy.SOURCE)
-        public @interface StatusValues {}
 
         /**
          * This state indicates that the request is completed, canceled, or is not
@@ -517,136 +336,34 @@ public interface UrlRequest {
         public static final int READING_RESPONSE = 14;
 
         private Status() {}
-
-        /**
-         * Convert a LoadState int to one of values listed above.
-         * @param loadState a LoadState to convert.
-         * @return static int Status.
-         * @hide only used by internal implementation.
-         */
-        @StatusValues
-        public static int convertLoadState(int loadState) {
-            assert loadState >= LoadState.IDLE && loadState <= LoadState.READING_RESPONSE;
-            switch (loadState) {
-                case (LoadState.IDLE):
-                    return IDLE;
-
-                case (LoadState.WAITING_FOR_STALLED_SOCKET_POOL):
-                    return WAITING_FOR_STALLED_SOCKET_POOL;
-
-                case (LoadState.WAITING_FOR_AVAILABLE_SOCKET):
-                    return WAITING_FOR_AVAILABLE_SOCKET;
-
-                case (LoadState.WAITING_FOR_DELEGATE):
-                    return WAITING_FOR_DELEGATE;
-
-                case (LoadState.WAITING_FOR_CACHE):
-                    return WAITING_FOR_CACHE;
-
-                case (LoadState.DOWNLOADING_PROXY_SCRIPT):
-                    return DOWNLOADING_PROXY_SCRIPT;
-
-                case (LoadState.RESOLVING_PROXY_FOR_URL):
-                    return RESOLVING_PROXY_FOR_URL;
-
-                case (LoadState.RESOLVING_HOST_IN_PROXY_SCRIPT):
-                    return RESOLVING_HOST_IN_PROXY_SCRIPT;
-
-                case (LoadState.ESTABLISHING_PROXY_TUNNEL):
-                    return ESTABLISHING_PROXY_TUNNEL;
-
-                case (LoadState.RESOLVING_HOST):
-                    return RESOLVING_HOST;
-
-                case (LoadState.CONNECTING):
-                    return CONNECTING;
-
-                case (LoadState.SSL_HANDSHAKE):
-                    return SSL_HANDSHAKE;
-
-                case (LoadState.SENDING_REQUEST):
-                    return SENDING_REQUEST;
-
-                case (LoadState.WAITING_FOR_RESPONSE):
-                    return WAITING_FOR_RESPONSE;
-
-                case (LoadState.READING_RESPONSE):
-                    return READING_RESPONSE;
-
-                default:
-                    // A load state is retrieved but there is no corresponding
-                    // request status. This most likely means that the mapping is
-                    // incorrect.
-                    throw new IllegalArgumentException("No request status found.");
-            }
-        }
     }
 
     /**
      * Listener class used with {@link #getStatus} to receive the status of a
      * {@link UrlRequest}.
      */
-    public abstract class StatusListener {
+    public abstract static class StatusListener {
         /**
          * Invoked on {@link UrlRequest}'s {@link Executor}'s thread when request
          * status is obtained.
          * @param status integer representing the status of the request. It is
          *         one of the values defined in {@link Status}.
          */
-        public abstract void onStatus(@Status.StatusValues int status);
+        public abstract void onStatus(int status);
     }
 
     /**
-     * Sets the HTTP method verb to use for this request. Must be done before
-     * request has started.
-     *
-     * <p>The default when this method is not called is "GET" if the request has
-     * no body or "POST" if it does.
-     *
-     * @param method "GET", "HEAD", "DELETE", "POST" or "PUT".
-     * @deprecated Use {@link Builder#setHttpMethod}.
-     * @hide
-     */
-    @Deprecated public void setHttpMethod(String method);
-
-    /**
-     * Adds a request header. Must be done before request has started.
-     *
-     * @param header header name.
-     * @param value header value.
-     * @deprecated Use {@link Builder#setPriority}.
-     * @hide
-     */
-    @Deprecated public void addHeader(String header, String value);
-
-    /**
-     * Sets upload data provider. Must be done before request has started. May only be
-     * invoked once per request. Switches method to "POST" if not explicitly
-     * set. Starting the request will throw an exception if a Content-Type
-     * header is not set.
-     *
-     * @param uploadDataProvider responsible for providing the upload data.
-     * @param executor All {@code uploadDataProvider} methods will be invoked
-     *     using this {@code Executor}. May optionally be the same
-     *     {@code Executor} the request itself is using.
-     * @deprecated Use {@link Builder#setUploadDataProvider}.
-     * @hide
-     */
-    @Deprecated
-    public void setUploadDataProvider(UploadDataProvider uploadDataProvider, Executor executor);
-
-    /**
      * Starts the request, all callbacks go to {@link Callback}. May only be called
      * once. May not be called if {@link #cancel} has been called.
      */
-    public void start();
+    public abstract void start();
 
     /**
      * Follows a pending redirect. Must only be called at most once for each
      * invocation of {@link Callback#onRedirectReceived
      * onRedirectReceived()}.
      */
-    public void followRedirect();
+    public abstract void followRedirect();
 
     /**
      * Attempts to read part of the response body into the provided buffer.
@@ -665,7 +382,7 @@ public interface UrlRequest {
      *     position, limit, or data between its position and limit until the
      *     request calls back into the {@link Callback}.
      */
-    public void read(ByteBuffer buffer);
+    public abstract void read(ByteBuffer buffer);
 
     /**
      * Cancels the request. Can be called at any time.
@@ -679,7 +396,7 @@ public interface UrlRequest {
      * {@code cancel()} is called. Otherwise, at most one callback method may be
      * invoked after {@code cancel()} has completed.
      */
-    public void cancel();
+    public abstract void cancel();
 
     /**
      * Returns {@code true} if the request was successfully started and is now
@@ -687,7 +404,7 @@ public interface UrlRequest {
      * @return {@code true} if the request was successfully started and is now
      *         finished (completed, canceled, or failed).
      */
-    public boolean isDone();
+    public abstract boolean isDone();
 
     /**
      * Queries the status of the request.
@@ -696,7 +413,7 @@ public interface UrlRequest {
      *         back on the {@link Executor} passed in when the request was
      *         created.
      */
-    public void getStatus(final StatusListener listener);
+    public abstract void getStatus(final StatusListener listener);
 
     // Note:  There are deliberately no accessors for the results of the request
     // here. Having none removes any ambiguity over when they are populated,