Enable NQE when Cronet Engine is built

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.annotation.SuppressLint;
 import android.content.Context;
 import android.net.http.HttpResponseCache;
 import android.support.annotation.IntDef;
@@ skipping 101 matching lines @@
         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;
+        private boolean mNetworkQualityEstimatorEnabled;
+        private Executor mNetworkQualityExecutor;
 
         /**
          * Default config enables SPDY, disables QUIC, SDCH and HTTP cache.
          * @param context Android {@link Context} for engine to use.
          */
         public Builder(Context context) {
             mContext = context;
             setLibraryName("cronet");
             enableLegacyMode(false);
             enableQUIC(false);
             enableHTTP2(true);
             enableSDCH(false);
             enableHttpCache(HTTP_CACHE_DISABLED, 0);
+            enableNetworkQualityEstimator(false, null);
         }
 
         /**
          * Constructs a User-Agent string including application name and version,
          * system build version, model and id, and Cronet version.
          *
          * @return User-Agent string.
          */
         public String getDefaultUserAgent() {
             return UserAgent.from(mContext);
@@ skipping 449 matching lines @@
         Builder setMockCertVerifierForTesting(long mockCertVerifier) {
             mMockCertVerifier = mockCertVerifier;
             return this;
         }
 
         long mockCertVerifier() {
             return mMockCertVerifier;
         }
 
         /**
+         * Enables the network quality estimator, which collects and reports
+         * measurements of round trip time (RTT) and downstream throughput at
+         * various layers of the network stack. After enabling the estimator,
+         * listeners of RTT and throughput can be added with
+         * {@link #addRttListener} and {@link #addThroughputListener} and
+         * removed with {@link #removeRttListener} and
+         * {@link #removeThroughputListener}. The estimator uses memory and CPU
+         * only when enabled.
+         * @param value {@code true} to enable network quality estimator,
+         *            {@code false} to disable.
+         * @param executor an executor that will be used to notified all

[bengr, 2016/06/07 20:53:16]

nit: notified -> notify

[tbansal1, 2016/06/07 21:37:16]

Done.

+         *            added RTT and throughput listeners.
+         * @hide as it's a prototype.
+         * @return the builder to facilitate chaining.
+         */
+        public Builder enableNetworkQualityEstimator(boolean value, Executor executor) {

[bengr, 2016/06/07 20:53:16]

Why do consumers need to call this when they don't want NQE? I.e., wouldn't the
following suffice?

Builder enabledNetworkQualityEstimator(Executor executor)

[tbansal1, 2016/06/07 21:37:16]

I kept it consistent with other CronetEngine.Builder calls. e.g., enableQuic(),
enableSDCH() all take a boolean value. I suppose it makes it slightly easier to
disable the feature.

[bengr, 2016/06/07 22:55:12]

This seems like a really bad way to do things. Please check with a cronet owner
before landing this.

[tbansal1, 2016/06/08 01:10:33]

Sure, but I think this is sensible. e.g., in future NQE (or any other
experimental feature such as QUIC) may be enabled  by default, and it may
automatically adjust network timeouts based on network quality. This API gives
an option for embedders to disable NQE (and automatic adjustment of timeouts) if
they do not want.

This would be similar to how HTTP2 is currently enabled by default in Cronet but
the API enableHttp2(boolean) gives an option to the embedder to disable it, if
they don't want to use HTTP2.

+            mNetworkQualityEstimatorEnabled = value;
+            mNetworkQualityExecutor = executor;
+            if (mNetworkQualityEstimatorEnabled && mNetworkQualityExecutor == null) {

[bengr, 2016/06/07 20:53:16]

With the new prototype, the first condition here should be removed.

[tbansal1, 2016/06/07 21:37:16]

I did not switch to the new prototype, so this is not done.

+                throw new IllegalStateException("Network quality executor must not be NULL");
+            }
+            return this;
+        }
+
+        /**
+         * Returns if network quality estimator has been enabled for this builder.

[bengr, 2016/06/07 20:53:16]

Returns if -> Returns true if the

[tbansal1, 2016/06/07 21:37:16]

Done.

+         * @hide as it's a prototype.
+         */
+        boolean networkQualityEstimatorEnabled() {
+            return mNetworkQualityEstimatorEnabled;
+        }
+
+        /**
+         * Returns the executor for the network quality estimator for this builder.
+         * @hide as it's a prototype.
+         */
+        Executor networkQualityEstimatorExecutor() {
+            return mNetworkQualityExecutor;
+        }
+
+        /**
          * Returns {@link Context} for builder.
          *
          * @return {@link Context} for builder.
          */
         Context getContext() {
             return mContext;
         }
 
         /**
          * Build a {@link CronetEngine} using this builder's configuration.
@@ skipping 171 matching lines @@
      * @return differences in metrics collected by Cronet, since the last call
      *         to {@code getGlobalMetricsDeltas()}, serialized as a
      *         <a href=https://developers.google.com/protocol-buffers>protobuf
      *         </a>.
      */
     public abstract byte[] getGlobalMetricsDeltas();
 
     /**
      * Enables the network quality estimator, which collects and reports
      * measurements of round trip time (RTT) and downstream throughput at
-     * various layers of the network stack. After enabling the estimator,
-     * listeners of RTT and throughput can be added with
+     * various layers of the network stack. This API has been deprecated in
+     * favor of CronetEngineBuilder.enableNetworkQualityEstimator.
+     * After enabling the estimator, listeners of RTT and throughput can be
+     * added with
      * {@link #addRttListener} and {@link #addThroughputListener} and
      * removed with {@link #removeRttListener} and
      * {@link #removeThroughputListener}. The estimator uses memory and CPU
      * only when enabled.
      * @param executor an executor that will be used to notified all
      *            added RTT and throughput listeners.
      * @hide as it's a prototype.
+     * @hide TODO(tbansal):  http://crbug.com/618034 Remove this API once all
+     * consumers have switched to using
+     * CronetEngineBuilder.enableNetworkQualityEstimator.
      */
     public abstract void enableNetworkQualityEstimator(Executor executor);
 
     /**
-     * Enables the network quality estimator for testing. This must be called
-     * before round trip time and throughput listeners are added. Set both
-     * boolean parameters to false for default behavior.
+     * Configures the network quality estimator for testing. This must be called
+     * before round trip time and throughput listeners are added, and after the
+     * network quality estimator has been enabled.
      * @param useLocalHostRequests include requests to localhost in estimates.
-     * @param useSmallerResponses include small responses in throughput estimates.
-     * @param executor an {@link java.util.concurrent.Executor} on which all
-     *            listeners will be called.
+     * @param useSmallerResponses include small responses in throughput
+     * estimates.
      * @hide as it's a prototype.
      */
-    abstract void enableNetworkQualityEstimatorForTesting(
-            boolean useLocalHostRequests, boolean useSmallerResponses, Executor executor);
+    abstract void configureNetworkQualityEstimatorForTesting(
+            boolean useLocalHostRequests, boolean useSmallerResponses);
 
     /**
      * Registers a listener that gets called whenever the network quality
      * estimator witnesses a sample round trip time. This must be called
      * after {@link #enableNetworkQualityEstimator}, and with throw an
      * exception otherwise. Round trip times may be recorded at various layers
      * of the network stack, including TCP, QUIC, and at the URL request layer.
      * The listener is called on the {@link java.util.concurrent.Executor} that
      * is passed to {@link #enableNetworkQualityEstimator}.
      * @param listener the listener of round trip times.
      * @hide as it's a prototype.
      */
     public abstract void addRttListener(NetworkQualityRttListener listener);
 
     /**
      * Removes a listener of round trip times if previously registered with
      * {@link #addRttListener}. This should be called after a
      * {@link NetworkQualityRttListener} is added in order to stop receiving
      * observations.
      * @param listener the listener of round trip times.
      * @hide as it's a prototype.
      */
     public abstract void removeRttListener(NetworkQualityRttListener listener);
 
     /**
      * Registers a listener that gets called whenever the network quality
      * estimator witnesses a sample throughput measurement. This must be called
-     * after {@link #enableNetworkQualityEstimator}. Throughput observations
+     * after network quality estimator has been enabled. Throughput observations

[bengr, 2016/06/07 20:53:16]

network -> the network

[tbansal1, 2016/06/07 21:37:16]

Done.

      * are computed by measuring bytes read over the active network interface
      * at times when at least one URL response is being received. The listener
      * is called on the {@link java.util.concurrent.Executor} that is passed to
      * {@link #enableNetworkQualityEstimator}.
      * @param listener the listener of throughput.
      * @hide as it's a prototype.
      */
     public abstract void addThroughputListener(NetworkQualityThroughputListener listener);
 
     /**
@@ skipping 106 matching lines @@
             // Leave as null.
         } catch (Exception e) {
             throw new IllegalStateException("Cannot instantiate: " + CRONET_URL_REQUEST_CONTEXT, e);
         }
         return cronetEngine;
     }
 
     /**
      * Registers a listener that gets called after the end of each request with the request info.
      *
-     * <p>This must be called after {@link #enableNetworkQualityEstimator} and will throw an
-     * exception otherwise.
+     * <p>This must be called after network quality estimator has been enabled.

[bengr, 2016/06/07 20:53:16]

after -> after the

[tbansal1, 2016/06/07 21:37:16]

Done.

      *
      * <p>The listener is called on the {@link java.util.concurrent.Executor} that
      * is passed to {@link #enableNetworkQualityEstimator}.
      *
      * @param listener the listener for finished requests.
      *
      * @hide as it's a prototype.
      */
     public abstract void addRequestFinishedListener(RequestFinishedListener listener);

[bengr, 2016/06/07 20:53:16]

This should throw an exception if nqe is not enabled, right?

[tbansal1, 2016/06/07 21:37:16]

It does. Now, added the comment to make it clearer.

 
     /**
      * Removes a finished request listener.
      *
      * @param listener the listener to remove.
      *
      * @hide it's a prototype.
      */
     public abstract void removeRequestFinishedListener(RequestFinishedListener listener);
 
@@ skipping 116 matching lines @@
      * @hide as it's a prototype.
      */
     public interface RequestFinishedListener { // TODO(klm): Add a convenience abstract class.
         /**
          * Invoked with request info.
          * @param requestInfo {@link UrlRequestInfo} for finished request.
          */
         void onRequestFinished(UrlRequestInfo requestInfo);
     }
 }