Cronet API Refactoring

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

 // Copyright 2014 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.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}.
+ * Created using {@link CronetEngine#newUrlRequestBuilder(String, Callback, Executor)}.

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

you replaced a type with a method that returns that type, I think you might want
to go back to the type as the method you reference does not create a UrlRequest
but the builder for it instead.

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

Done.

  * Note: All methods must be called on the {@link Executor} passed in during creation.

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

in during creation->to {@link CronetEngine#newUrlRequestBuilder}

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

Done.

  */
-public interface UrlRequest {
+public abstract class UrlRequest {
     /**
      * Builder for {@link UrlRequest}s. Allows configuring requests before constructing them
      * with {@link Builder#build}.

[pauljensen, 2016/09/27 12:52:32]

We need a sentence explaining how to create a UrlRequest.Builder, something
like:
Created by {@link CronetEngine#newUrlRequestBuilder}.

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

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.
          *
          * <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".
          * @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.
          *
          * @param header header name.
          * @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 {}

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

how come you removed this?

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

I moved it to CronetEngineBase since it is used there. Should we make
@RequestPriority annotation public and add it to the Javadoc; othwrwise, it has
limited use in the API?

+        public abstract Builder disableCache();
 
         /**
          * Lowest request priority. Passed to {@link #setPriority}.
          */
         public static final int REQUEST_PRIORITY_IDLE = 0;
         /**
          * Very low request priority. Passed to {@link #setPriority}.
          */
         public static final int REQUEST_PRIORITY_LOWEST = 1;
         /**
          * Low request priority. Passed to {@link #setPriority}.
          */
         public static final int REQUEST_PRIORITY_LOW = 2;
         /**
          * Medium request priority. Passed to {@link #setPriority}. This is the
          * default priority given to the request.
          */
         public static final int REQUEST_PRIORITY_MEDIUM = 3;
         /**
          * Highest request priority. Passed to {@link #setPriority}.
          */
         public static final int REQUEST_PRIORITY_HIGHEST = 4;
 
         /**
          * 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.
          *
          * @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
          * 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.
          * @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
          * {@code UploadDataProvider}s and {@code UrlRequest.Callback}s) is intentionally performing
          * inline execution, like Guava's directExecutor or
          * {@link java.util.concurrent.ThreadPoolExecutor.CallerRunsPolicy}.
          *
          * <p><b>Warning:</b> This option makes it easy to accidentally block the network thread.
          * 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
          * {@link Builder}. The returned {@code UrlRequest} can then be started
          * by calling {@link UrlRequest#start}.
          *
          * @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();
     }
 
     /**
      * Users of Cronet extend this class to receive callbacks indicating the
      * progress of a {@link UrlRequest} being processed. An instance of this class
      * is passed in to {@link UrlRequest.Builder}'s constructor when
      * constructing the {@code UrlRequest}.
      * <p>
      * Note:  All methods will be invoked on the thread of the
      * {@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
          * {@link Callback#onResponseStarted onResponseStarted()}.
          * The body of the redirect response, if it has one, will be ignored.
          *
          * The redirect will not be followed until the URLRequest's
          * {@link UrlRequest#followRedirect} method is called, either
          * synchronously or asynchronously.
          *
@@ skipping 83 matching lines @@
          * @param info Response information. May be {@code null} if no response was
          *         received.
          */
         public void onCanceled(UrlRequest request, UrlResponseInfo info) {}
     }
 
     /**
      * 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 {}

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

ditto?

 
         /**
          * This state indicates that the request is completed, canceled, or is not
          * started.
          */
         public static final int INVALID = -1;
         /**
          * This state corresponds to a resource load that has either not yet begun
          * or is idle waiting for the consumer to do something to move things along
          * (e.g. when the consumer of a {@link UrlRequest} has not called
@@ skipping 83 matching lines @@
         /**
          * This state corresponds to a resource load that is blocked waiting for a
          * read to complete. In the case of a HTTP transaction, this corresponds to
          * the period after the response headers have been received and before all
          * of the response body has been downloaded. (NOTE: This state only applies
          * for an {@link UrlRequest} while there is an outstanding
          * {@link UrlRequest#read read()} operation.)
          */
         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.");
-            }
-        }
+        protected Status() {}

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

why the transition to protected?

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

The status is defined in the public API (org.chromium.net pacgkage) but is
extended by UrlRequestBase.Status which is in the impl package. I am not really
sure if we should extend it.

[pauljensen, 2016/09/27 19:08:09]

Maybe just not extend Status in the impl.

[kapishnikov, 2016/09/28 00:20:49]

Done.

     }
 
     /**
      * 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.
      * Must only be called at most once in response to each invocation of the
      * {@link Callback#onResponseStarted onResponseStarted()} and {@link
      * Callback#onReadCompleted onReadCompleted()} methods of the {@link
      * Callback}. Each call will result in an asynchronous call to
      * either the {@link Callback Callback's}
      * {@link Callback#onReadCompleted onReadCompleted()} method if data
      * is read, its {@link Callback#onSucceeded onSucceeded()} method if
      * there's no more data to read, or its {@link Callback#onFailed
      * onFailed()} method if there's an error.
      *
      * @param buffer {@link ByteBuffer} to write response body to. Must be a
      *     direct ByteBuffer. The embedder must not read or modify buffer's
      *     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.
      * {@link Callback#onCanceled onCanceled()} will be invoked when cancellation
      * is complete and no further callback methods will be invoked. If the
      * request has completed or has not started, calling {@code cancel()} has no
      * effect and {@code onCanceled()} will not be invoked. If the
      * {@link Executor} passed in during {@code UrlRequest} construction runs
      * tasks on a single thread, and {@code cancel()} is called on that thread,
      * no callback methods (besides {@code onCanceled()}) will be invoked after
      * {@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
      * finished (completed, canceled, or failed).
      * @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.
      * @param listener a {@link StatusListener} that will be invoked with
      *         the request's current status. {@code listener} will be invoked
      *         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,
     // particularly in the redirect case.
 }