[Cronet] Remove JSON serialization of CronetEngine.Builder

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

Index: components/cronet/android/api/src/org/chromium/net/CronetEngine.java
diff --git a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
index 6e7d1e0940ccc2fa4668cc24373cd866e90866ea..87fb54dabb301749187a99d49658965017fb6cc4 100644
--- a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
+++ b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
@@ -6,12 +6,9 @@ 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;
@@ -20,6 +17,7 @@ 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;
@@ -35,16 +33,50 @@ public abstract class CronetEngine {
      * 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);
@@ -67,11 +99,12 @@ public abstract class CronetEngine {
          * @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;
         }
 
         /**
@@ -90,12 +123,12 @@ public abstract class CronetEngine {
                 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;
         }
 
         /**
@@ -107,11 +140,12 @@ public abstract class CronetEngine {
          */
         @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;
         }
 
         /**
@@ -119,11 +153,12 @@ public abstract class CronetEngine {
          * @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;
         }
 
         /**
@@ -132,7 +167,12 @@ public abstract class CronetEngine {
          * @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;
         }
 
         /**
@@ -141,7 +181,12 @@ public abstract class CronetEngine {
          * @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;
         }
 
         /**
@@ -152,7 +197,12 @@ public abstract class CronetEngine {
          * @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;
         }
 
         /**
@@ -163,7 +213,12 @@ public abstract class CronetEngine {
          * @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;
         }
 
         /**
@@ -189,13 +244,24 @@ public abstract class CronetEngine {
                 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,
@@ -241,34 +307,47 @@ public abstract class CronetEngine {
          */
         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}
@@ -284,24 +363,14 @@ public abstract class CronetEngine {
             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);
-                }
-
-                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.
-            }
+            mQuicHints.add(new QuicHint(host, port, alternatePort));
             return this;
         }
 
+        List<QuicHint> quicHints() {
+            return mQuicHints;
+        }
+
         /**
          * Sets experimental options to be used in Cronet.
          *
@@ -309,22 +378,24 @@ public abstract class CronetEngine {
          * @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;
         }
 
-        /**
-         * Gets a JSON string representation of the builder.
-         */
-        String toJSONString() {
-            return mConfig.toString();
+        long mockCertVerifier() {
+            return mMockCertVerifier;
         }
 
         /**
@@ -337,55 +408,13 @@ public abstract class CronetEngine {
         }
 
         /**
-         * 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;
         }
     }
@@ -661,7 +690,7 @@ public abstract class CronetEngine {
     @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()) {