[Cronet] Remove JSON serialization of CronetEngine.Builder

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

 // Copyright 2015 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.IntDef;
+import android.text.TextUtils;
 import android.util.Log;
 
-import org.json.JSONArray;
-import org.json.JSONException;
-import org.json.JSONObject;
-
 import java.io.File;
 import java.lang.annotation.Retention;
 import java.lang.annotation.RetentionPolicy;
 import java.lang.reflect.Constructor;
 import java.net.Proxy;
 import java.net.URL;
 import java.net.URLConnection;
 import java.net.URLStreamHandlerFactory;
+import java.util.LinkedList;
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Executor;
 
 /**
  * An engine to process {@link UrlRequest}s, which uses the best HTTP stack
  * available on the current platform.
  */
 public abstract class CronetEngine {
     /**
      * A builder for {@link CronetEngine}s, which allows runtime configuration of
      * {@code CronetEngine}. Configuration options are set on the builder and
      * then {@link #build} is called to create the {@code CronetEngine}.
      */
     public static class Builder {
-        private final JSONObject mConfig;
+        // A hint that a host supports QUIC.
+        static class QuicHint {
+            // The host.
+            final String mHost;
+            // Port of the server that supports QUIC.
+            final int mPort;
+            // Alternate protocol port.
+            final int mAlternatePort;
+
+            QuicHint(String host, int port, int alternatePort) {
+                mHost = host;
+                mPort = port;
+                mAlternatePort = alternatePort;
+            }
+        }
+
+        // Private fields are simply storage of configuration for the resulting CronetEngine.
+        // See setters below for verbose descriptions.
         private final Context mContext;
+        private final List<QuicHint> mQuicHints = new LinkedList<QuicHint>();
+        private String mUserAgent;
+        private String mStoragePath;
+        private boolean mLegacyModeEnabled;
+        private String mLibraryName;
+        private boolean mQuicEnabled;
+        private boolean mHttp2Enabled;
+        private boolean mSdchEnabled;
+        private String mDataReductionProxyKey;
+        private String mDataReductionProxyPrimaryProxy;
+        private String mDataReductionProxyFallbackProxy;
+        private String mDataReductionProxySecureProxyCheckUrl;
+        private boolean mDisableCache;
+        private int mHttpCacheMode;
+        private long mHttpCacheMaxSize;
+        private String mExperimentalOptions;
+        private long mMockCertVerifier;
 
         /**
          * Default config enables SPDY, disables QUIC, SDCH and HTTP cache.
          * @param context Android {@link Context} for engine to use.
          */
         public Builder(Context context) {
-            mConfig = new JSONObject();
             mContext = context;
+            setLibraryName("cronet");
             enableLegacyMode(false);
             enableQUIC(false);
             enableHTTP2(true);
             enableSDCH(false);
             enableHttpCache(HTTP_CACHE_DISABLED, 0);
         }
 
         /**
          * Constructs a User-Agent string including Cronet version, and
          * application name and version.
          *
          * @return User-Agent string.
          */
         public String getDefaultUserAgent() {
             return UserAgent.from(mContext);
         }
 
         /**
          * Overrides the User-Agent header for all requests.
          * @return the builder to facilitate chaining.
          */
         public Builder setUserAgent(String userAgent) {
-            return putString(CronetEngineBuilderList.USER_AGENT, userAgent);
+            mUserAgent = userAgent;
+            return this;
         }
 
         String getUserAgent() {
-            return mConfig.optString(CronetEngineBuilderList.USER_AGENT);
+            return mUserAgent;
         }
 
         /**
          * Sets directory for HTTP Cache and Cookie Storage. The directory must
          * exist.
          * <p>
          * <b>NOTE:</b> Do not use the same storage directory with more than one
          * {@code CronetEngine} at a time. Access to the storage directory does
          * not support concurrent access by multiple {@code CronetEngine}s.
          *
          * @param value path to existing directory.
          * @return the builder to facilitate chaining.
          */
         public Builder setStoragePath(String value) {
             if (!new File(value).isDirectory()) {
                 throw new IllegalArgumentException(
                         "Storage path must be set to existing directory");
             }
-
-            return putString(CronetEngineBuilderList.STORAGE_PATH, value);
+            mStoragePath = value;
+            return this;
         }
 
         String storagePath() {
-            return mConfig.optString(CronetEngineBuilderList.STORAGE_PATH);
+            return mStoragePath;
         }
 
         /**
          * Sets whether falling back to implementation based on system's
          * {@link java.net.HttpURLConnection} implementation is enabled.
          * Defaults to disabled.
          * @return the builder to facilitate chaining.
          * @deprecated Not supported by the new API.
          */
         @Deprecated
         public Builder enableLegacyMode(boolean value) {
-            return putBoolean(CronetEngineBuilderList.ENABLE_LEGACY_MODE, value);
+            mLegacyModeEnabled = value;
+            return this;
         }
 
         boolean legacyMode() {
-            return mConfig.optBoolean(CronetEngineBuilderList.ENABLE_LEGACY_MODE);
+            return mLegacyModeEnabled;
         }
 
         /**
          * Overrides the name of the native library backing Cronet.
          * @return the builder to facilitate chaining.
          */
         Builder setLibraryName(String libName) {
-            return putString(CronetEngineBuilderList.NATIVE_LIBRARY_NAME, libName);
+            mLibraryName = libName;
+            return this;
         }
 
         String libraryName() {
-            return mConfig.optString(CronetEngineBuilderList.NATIVE_LIBRARY_NAME, "cronet");
+            return mLibraryName;
         }
 
         /**
          * Sets whether <a href="https://www.chromium.org/quic">QUIC</a> protocol
          * is enabled. Defaults to disabled.
          * @return the builder to facilitate chaining.
          */
         public Builder enableQUIC(boolean value) {
-            return putBoolean(CronetEngineBuilderList.ENABLE_QUIC, value);
+            mQuicEnabled = value;
+            return this;
+        }
+
+        boolean quicEnabled() {
+            return mQuicEnabled;
         }
 
         /**
          * Sets whether <a href="https://tools.ietf.org/html/rfc7540">HTTP/2</a>
          * protocol is enabled. Defaults to enabled.
          * @return the builder to facilitate chaining.
          */
         public Builder enableHTTP2(boolean value) {
-            return putBoolean(CronetEngineBuilderList.ENABLE_SPDY, value);
+            mHttp2Enabled = value;
+            return this;
+        }
+
+        boolean http2Enabled() {
+            return mHttp2Enabled;
         }
 
         /**
          * Sets whether
          * <a
          * href="https://lists.w3.org/Archives/Public/ietf-http-wg/2008JulSep/att-0441/Shared_Dictionary_Compression_over_HTTP.pdf">
          * SDCH</a> compression is enabled. Defaults to disabled.
          * @return the builder to facilitate chaining.
          */
         public Builder enableSDCH(boolean value) {
-            return putBoolean(CronetEngineBuilderList.ENABLE_SDCH, value);
+            mSdchEnabled = value;
+            return this;
+        }
+
+        boolean sdchEnabled() {
+            return mSdchEnabled;
         }
 
         /**
          * Enables
          * <a href="https://developer.chrome.com/multidevice/data-compression">Data
          * Reduction Proxy</a>. Defaults to disabled.
          * @param key key to use when authenticating with the proxy.
          * @return the builder to facilitate chaining.
          */
         public Builder enableDataReductionProxy(String key) {
-            return (putString(CronetEngineBuilderList.DATA_REDUCTION_PROXY_KEY, key));
+            mDataReductionProxyKey = key;
+            return this;
+        }
+
+        String dataReductionProxyKey() {
+            return mDataReductionProxyKey;
         }
 
         /**
          * Overrides
          * <a href="https://developer.chrome.com/multidevice/data-compression">
          * Data Reduction Proxy</a> configuration parameters with a primary
          * proxy name, fallback proxy name, and a secure proxy check URL. Proxies
          * are specified as [scheme://]host[:port]. Used for testing.
          * @param primaryProxy the primary data reduction proxy to use.
          * @param fallbackProxy a fallback data reduction proxy to use.
          * @param secureProxyCheckUrl a URL to fetch to determine if using a secure
          * proxy is allowed.
          * @return the builder to facilitate chaining.
          * @hide
          * @deprecated Marked as deprecated because @hide doesn't properly hide but
          *         javadocs are built with nodeprecated="yes".
          */
         @SuppressWarnings("DepAnn")
         public Builder setDataReductionProxyOptions(
                 String primaryProxy, String fallbackProxy, String secureProxyCheckUrl) {
             if (primaryProxy.isEmpty() || fallbackProxy.isEmpty()
                     || secureProxyCheckUrl.isEmpty()) {
                 throw new IllegalArgumentException(
                         "Primary and fallback proxies and check url must be set");
             }
-            putString(CronetEngineBuilderList.DATA_REDUCTION_PRIMARY_PROXY, primaryProxy);
-            putString(CronetEngineBuilderList.DATA_REDUCTION_FALLBACK_PROXY, fallbackProxy);
-            putString(CronetEngineBuilderList.DATA_REDUCTION_SECURE_PROXY_CHECK_URL,
-                    secureProxyCheckUrl);
+            mDataReductionProxyPrimaryProxy = primaryProxy;
+            mDataReductionProxyFallbackProxy = fallbackProxy;
+            mDataReductionProxySecureProxyCheckUrl = secureProxyCheckUrl;
             return this;
         }
 
+        String dataReductionProxyPrimaryProxy() {
+            return mDataReductionProxyPrimaryProxy;
+        }
+
+        String dataReductionProxyFallbackProxy() {
+            return mDataReductionProxyFallbackProxy;
+        }
+
+        String dataReductionProxySecureProxyCheckUrl() {
+            return mDataReductionProxySecureProxyCheckUrl;
+        }
+
         /** @deprecated not really deprecated but hidden. */
         @IntDef({
                 HTTP_CACHE_DISABLED, HTTP_CACHE_IN_MEMORY, HTTP_CACHE_DISK_NO_HTTP, HTTP_CACHE_DISK,
         })
         @Retention(RetentionPolicy.SOURCE)
         @SuppressWarnings("DepAnn")
         public @interface HttpCacheSetting {}
 
         /**
          * Setting to disable HTTP cache. Some data may still be temporarily stored in memory.
@@ skipping 25 matching lines @@
          * Enables or disables caching of HTTP data and other information like QUIC
          * server information.
          * @param cacheMode control location and type of cached data. Must be one of
          *         {@link #HTTP_CACHE_DISABLED HTTP_CACHE_*}.
          * @param maxSize maximum size in bytes used to cache data (advisory and maybe
          * exceeded at times).
          * @return the builder to facilitate chaining.
          */
         public Builder enableHttpCache(@HttpCacheSetting int cacheMode, long maxSize) {
             if (cacheMode == HTTP_CACHE_DISK || cacheMode == HTTP_CACHE_DISK_NO_HTTP) {
-                if (storagePath().isEmpty()) {
+                if (TextUtils.isEmpty(storagePath())) {

[xunjieli, 2015/12/02 13:06:46]

I don't understand why we need TextUtils.isEmpty. If storagePath is set, then it
is not null. Can we test null instead?

[pauljensen, 2015/12/02 18:11:09]

Done.  I was using TextUtils.isEmpty() to match the old code (which uses
.isEmpty()) but that is no longer necessary.

                     throw new IllegalArgumentException("Storage path must be set");
                 }
             } else {
-                if (!storagePath().isEmpty()) {
+                if (!TextUtils.isEmpty(storagePath())) {
                     throw new IllegalArgumentException("Storage path must be empty");
                 }
             }
-            putBoolean(CronetEngineBuilderList.LOAD_DISABLE_CACHE,
-                    cacheMode == HTTP_CACHE_DISABLED || cacheMode == HTTP_CACHE_DISK_NO_HTTP);
-            putLong(CronetEngineBuilderList.HTTP_CACHE_MAX_SIZE, maxSize);
+            mDisableCache =
+                    (cacheMode == HTTP_CACHE_DISABLED || cacheMode == HTTP_CACHE_DISK_NO_HTTP);
+            mHttpCacheMaxSize = maxSize;
 
             switch (cacheMode) {
                 case HTTP_CACHE_DISABLED:
-                    return putString(CronetEngineBuilderList.HTTP_CACHE,
-                            CronetEngineBuilderList.HTTP_CACHE_DISABLED);
+                    mHttpCacheMode = HttpCacheType.DISABLED;
+                    break;
                 case HTTP_CACHE_DISK_NO_HTTP:
                 case HTTP_CACHE_DISK:
-                    return putString(CronetEngineBuilderList.HTTP_CACHE,
-                            CronetEngineBuilderList.HTTP_CACHE_DISK);
-
+                    mHttpCacheMode = HttpCacheType.DISK;
+                    break;
                 case HTTP_CACHE_IN_MEMORY:
-                    return putString(CronetEngineBuilderList.HTTP_CACHE,
-                            CronetEngineBuilderList.HTTP_CACHE_MEMORY);
+                    mHttpCacheMode = HttpCacheType.MEMORY;
+                    break;
+                default:
+                    throw new IllegalArgumentException("Unknown cache mode");
             }
             return this;
         }
 
+        boolean cacheDisabled() {
+            return mDisableCache;
+        }
+
+        long httpCacheMaxSize() {
+            return mHttpCacheMaxSize;
+        }
+
+        int httpCacheMode() {
+            return mHttpCacheMode;
+        }
+
         /**
          * Adds hint that {@code host} supports QUIC.
          * Note that {@link #enableHttpCache enableHttpCache}
          * ({@link #HTTP_CACHE_DISK}) is needed to take advantage of 0-RTT
          * connection establishment between sessions.
          *
          * @param host hostname of the server that supports QUIC.
          * @param port host of the server that supports QUIC.
          * @param alternatePort alternate port to use for QUIC.
          * @return the builder to facilitate chaining.
          */
         public Builder addQuicHint(String host, int port, int alternatePort) {
             if (host.contains("/")) {
                 throw new IllegalArgumentException("Illegal QUIC Hint Host: " + host);
             }
-            try {
-                JSONArray quicHints = mConfig.optJSONArray(CronetEngineBuilderList.QUIC_HINTS);
-                if (quicHints == null) {
-                    quicHints = new JSONArray();
-                    mConfig.put(CronetEngineBuilderList.QUIC_HINTS, quicHints);
-                }
+            mQuicHints.add(new QuicHint(host, port, alternatePort));
+            return this;
+        }
 
-                JSONObject hint = new JSONObject();
-                hint.put(CronetEngineBuilderList.QUIC_HINT_HOST, host);
-                hint.put(CronetEngineBuilderList.QUIC_HINT_PORT, port);
-                hint.put(CronetEngineBuilderList.QUIC_HINT_ALT_PORT, alternatePort);
-                quicHints.put(hint);
-            } catch (JSONException e) {
-                // Intentionally do nothing.
-            }
-            return this;
+        List<QuicHint> quicHints() {
+            return mQuicHints;
         }
 
         /**
          * Sets experimental options to be used in Cronet.
          *
          * @param options JSON formatted experimental options.
          * @return the builder to facilitate chaining.
          */
         public Builder setExperimentalOptions(String options) {
-            return putString(CronetEngineBuilderList.EXPERIMENTAL_OPTIONS, options);
+            mExperimentalOptions = options;
+            return this;
+        }
+
+        String experimentalOptions() {
+            return mExperimentalOptions;
         }
 
         /**
          * Sets a native MockCertVerifier for testing.
          */
         Builder setMockCertVerifierForTesting(long mockCertVerifier) {
-            return putString(
-                    CronetEngineBuilderList.MOCK_CERT_VERIFIER, String.valueOf(mockCertVerifier));
+            mMockCertVerifier = mockCertVerifier;
+            return this;
+        }
+
+        long mockCertVerifier() {
+            return mMockCertVerifier;
         }
 
         /**
-         * Gets a JSON string representation of the builder.
-         */
-        String toJSONString() {
-            return mConfig.toString();
-        }
-
-        /**
          * Returns {@link Context} for builder.
          *
          * @return {@link Context} for builder.
          */
         Context getContext() {
             return mContext;
         }
 
         /**
-         * Sets a boolean value in the config. Returns a reference to the same
-         * config object, so you can chain put calls together.
-         * @return the builder to facilitate chaining.
-         */
-        private Builder putBoolean(String key, boolean value) {
-            try {
-                mConfig.put(key, value);
-            } catch (JSONException e) {
-                // Intentionally do nothing.
-            }
-            return this;
-        }
-
-        /**
-         * Sets a long value in the config. Returns a reference to the same
-         * config object, so you can chain put calls together.
-         * @return the builder to facilitate chaining.
-         */
-        private Builder putLong(String key, long value) {
-            try {
-                mConfig.put(key, value);
-            } catch (JSONException e) {
-                // Intentionally do nothing.
-            }
-            return this;
-        }
-
-        /**
-         * Sets a string value in the config. Returns a reference to the same
-         * config object, so you can chain put calls together.
-         * @return the builder to facilitate chaining.
-         */
-        private Builder putString(String key, String value) {
-            try {
-                mConfig.put(key, value);
-            } catch (JSONException e) {
-                // Intentionally do nothing.
-            }
-            return this;
-        }
-
-        /**
          * Build a {@link CronetEngine} using this builder's configuration.
          */
         public CronetEngine build() {
             CronetEngine engine = createContext(this);
             // Clear MOCK_CERT_VERIFIER reference if there is any, since
             // the ownership has been transferred to the engine.
-            mConfig.remove(CronetEngineBuilderList.MOCK_CERT_VERIFIER);
+            mMockCertVerifier = 0;
             return engine;
         }
     }
 
     private static final String TAG = "UrlRequestFactory";
     private static final String CRONET_URL_REQUEST_CONTEXT =
             "org.chromium.net.CronetUrlRequestContext";
 
     /**
      * Creates a {@link UrlRequest} object. All callbacks will
@@ skipping 255 matching lines @@
     /**
      * Creates a {@link CronetEngine} with the given {@link Builder}.
      *
      * @param builder builder to used for creating the CronetEngine instance.
      * @return the created CronetEngine instance.
      * @deprecated Use {@link CronetEngine.Builder}.
      */
     @Deprecated
     public static CronetEngine createContext(Builder builder) {
         CronetEngine cronetEngine = null;
-        if (builder.getUserAgent().isEmpty()) {
+        if (TextUtils.isEmpty(builder.getUserAgent())) {

[xunjieli, 2015/12/02 13:06:46]

Same here. Can we do null test? If the embedder wants an empty string as the
user agent, we should probably allow it, since that is done by an explicit user
action.

[pauljensen, 2015/12/02 18:11:08]

Done.  I was using TextUtils.isEmpty() to match the old code (which uses
.isEmpty()) but that is no longer necessary.

             builder.setUserAgent(builder.getDefaultUserAgent());
         }
         if (!builder.legacyMode()) {
             cronetEngine = createCronetEngine(builder);
         }
         if (cronetEngine == null) {
             // TODO(mef): Fallback to stub implementation. Once stub
             // implementation is available merge with createCronetFactory.
             cronetEngine = createCronetEngine(builder);
         }
@@ skipping 15 matching lines @@
                 cronetEngine = possibleEngine;
             }
         } catch (ClassNotFoundException e) {
             // Leave as null.
         } catch (Exception e) {
             throw new IllegalStateException("Cannot instantiate: " + CRONET_URL_REQUEST_CONTEXT, e);
         }
         return cronetEngine;
     }
 }