[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.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 (storagePath() == null) {

[xunjieli, 2015/12/02 18:55:35]

nit: since we are already inside the builder, can we use mStoragePath directly
(here and the next if-statement? Just to be consistent with other variables in
this method

[pauljensen, 2015/12/03 18:51:45]

enableHttpCache() is more-or-less a setter for mDisableCache and mHttpCacheMode
so it should certainly be accessing them directly.  mStoragePath versus
storagePath() I think is more of a style choice.  Some people think member
methods should always use accessors and others don't.  I checked all the style
guides I know of and none seemed to express an opinion.  Personally I like
always using setters and getters so as to preserve the abstraction layer.  I
think I'll leave this as is unless you feel strongly (which I assume not as this
was a nit).

[xunjieli, 2015/12/03 18:53:41]

sgtm since it isn't a new change.

                     throw new IllegalArgumentException("Storage path must be set");
                 }
             } else {
-                if (!storagePath().isEmpty()) {
+                if (storagePath() != null) {
                     throw new IllegalArgumentException("Storage path must be empty");

[xunjieli, 2015/12/02 18:55:35]

nit: maybe change "Storage path must be empty" -> "Storage path must not be set"

[pauljensen, 2015/12/03 18:51:45]

Done.

                 }
             }
-            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 (builder.getUserAgent() == null) {
             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;
     }
 }