[Cronet] Remove JSON serialization of CronetEngine.Builder

components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.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.os.Build;
 import android.os.ConditionVariable;
 import android.os.Handler;
 import android.os.Looper;
@@ skipping 55 matching lines @@
             new ObserverList<NetworkQualityRttListener>();
 
     @GuardedBy("mNetworkQualityLock")
     private final ObserverList<NetworkQualityThroughputListener> mThroughputListenerList =
             new ObserverList<NetworkQualityThroughputListener>();
 
     @UsedByReflection("CronetEngine.java")
     public CronetUrlRequestContext(CronetEngine.Builder builder) {
         CronetLibraryLoader.ensureInitialized(builder.getContext(), builder);
         nativeSetMinLogLevel(getLoggingLevel());
-        mUrlRequestContextAdapter = nativeCreateRequestContextAdapter(builder.toJSONString());
+        mUrlRequestContextAdapter =
+                nativeCreateRequestContextAdapter(createNativeUrlRequestContextConfig(builder));
         if (mUrlRequestContextAdapter == 0) {
             throw new NullPointerException("Context Adapter creation failed.");
         }
 
         // Init native Chromium URLRequestContext on main UI thread.
         Runnable task = new Runnable() {
             @Override
             public void run() {
                 synchronized (mLock) {
                     // mUrlRequestContextAdapter is guaranteed to exist until
                     // initialization on main and network threads completes and
                     // initNetworkThread is called back on network thread.
                     nativeInitRequestContextOnMainThread(mUrlRequestContextAdapter);
                 }
             }
         };
         // Run task immediately or post it to the UI thread.
         if (Looper.getMainLooper() == Looper.myLooper()) {
             task.run();
         } else {
             new Handler(Looper.getMainLooper()).post(task);
         }
     }
 
+    static long createNativeUrlRequestContextConfig(CronetEngine.Builder builder) {
+        final long urlRequestContextConfig = nativeCreateRequestContextConfig(
+                builder.getUserAgent(), builder.storagePath(), builder.quicEnabled(),
+                builder.http2Enabled(), builder.sdchEnabled(), builder.dataReductionProxyKey(),
+                builder.dataReductionProxyPrimaryProxy(), builder.dataReductionProxyFallbackProxy(),
+                builder.dataReductionProxySecureProxyCheckUrl(), builder.cacheDisabled(),
+                builder.httpCacheMode(), builder.httpCacheMaxSize(), builder.experimentalOptions(),
+                builder.mockCertVerifier());
+        for (Builder.QuicHint quicHint : builder.quicHints()) {
+            nativeAddQuicHint(urlRequestContextConfig, quicHint.mHost, quicHint.mPort,

[xunjieli, 2015/12/01 17:30:38]

I don't really like adding multiple JNI calls here. Can we consolidate them into
some structured info and pass it at once with the rest of the args?

[pauljensen, 2015/12/02 03:50:01]

I don't really like it either, but I'm not too worried about JNI as the overhead
is very low, like around 1 microsecond.
I don't like the alternatives any better:
1. could pass an array of classes but we'd have to use GetFieldID() which is
horribly slow as it uses reflection.
2. could pass arrays of strings (for the hosts) and arrays of ints (for the
ports) but that leads to a hideous looking API and AFAIK there is no handy
function for converting from Java arrays of strings to std::vectors of
std::strings.
I'm open to alternate suggestions.

+                    quicHint.mAlternatePort);
+        }
+        return urlRequestContextConfig;
+    }
+
     @Override
     public UrlRequest createRequest(String url, UrlRequest.Callback callback, Executor executor) {
         synchronized (mLock) {
             checkHaveAdapter();
             return new CronetUrlRequest(this, mUrlRequestContextAdapter, url,
                     UrlRequest.Builder.REQUEST_PRIORITY_MEDIUM, callback, executor);
         }
     }
 
     @Override
@@ skipping 282 matching lines @@
     void postObservationTaskToNetworkQualityExecutor(Runnable task) {
         try {
             mNetworkQualityExecutor.execute(task);
         } catch (RejectedExecutionException failException) {
             Log.e(CronetUrlRequestContext.LOG_TAG, "Exception posting task to executor",
                     failException);
         }
     }
 
     // Native methods are implemented in cronet_url_request_context_adapter.cc.
-    private static native long nativeCreateRequestContextAdapter(String config);
+    private static native long nativeCreateRequestContextConfig(String userAgent,
+            String storagePath, boolean quicEnabled, boolean http2Enabled, boolean sdchEnabled,
+            String dataReductionProxyKey, String dataReductionProxyPrimaryProxy,
+            String dataReductionProxyFallbackProxy, String dataReductionProxySecureProxyCheckUrl,
+            boolean disableCache, int httpCacheMode, long httpCacheMaxSize,
+            String experimentalOptions, long mockCertVerifier);
+
+    private static native void nativeAddQuicHint(
+            long urlRequestContextConfig, String host, int port, int alternatePort);
+
+    private static native long nativeCreateRequestContextAdapter(long urlRequestContextConfig);

[xunjieli, 2015/12/01 17:30:38]

Not sure whether we should consolidate nativeCreateRequestContextContext with
nativeCreateRequestContextAdapter, so we have one JNI call. That will sacrifice
readability. But the JNI slides do recommend against traversing the boundary
unnecessarily. Not sure if this counts.

[pauljensen, 2015/12/02 03:50:01]

I split it for a few reasons:
1. The ChromiumURLRequestContext and the CronetURLRequestContext both share the
Config JNI (this is why createNativeUrlRequestContextConfig() is package-private
not private), but then they create different adapters from the config.
2. the QUIC hints must be added piece by piece in between creating the Config
and the Adapter
3. for testing, I want to ensure I create the Config properly but I don't want
to have to put in mountains of mocking to actually get an Adapter, I just want
to check the Config fields are passed through properly
I'm open to alternate suggestions.

 
     private static native int nativeSetMinLogLevel(int loggingLevel);
 
     private static native byte[] nativeGetHistogramDeltas();
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeDestroy(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeStartNetLogToFile(long nativePtr,
             String fileName, boolean logAll);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeStopNetLog(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeInitRequestContextOnMainThread(long nativePtr);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeEnableNetworkQualityEstimator(
             long nativePtr, boolean useLocalHostRequests, boolean useSmallerResponses);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeProvideRTTObservations(long nativePtr, boolean should);
 
     @NativeClassQualifiedName("CronetURLRequestContextAdapter")
     private native void nativeProvideThroughputObservations(long nativePtr, boolean should);
 }