Enable NQE when Cronet Engine is built

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 f30991ac3c7ae2223aaaeabfa683902ab575467f..35972f0d3b55d0d7b2b69a9ee7b160e9c37b42cb 100644
--- a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
+++ b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
@@ -119,6 +119,8 @@ public abstract class CronetEngine {
         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.
@@ -132,6 +134,7 @@ public abstract class CronetEngine {
             enableHTTP2(true);
             enableSDCH(false);
             enableHttpCache(HTTP_CACHE_DISABLED, 0);
+            enableNetworkQualityEstimator(false, null);
         }
 
         /**
@@ -601,6 +604,47 @@ public abstract class CronetEngine {
         }
 
         /**
+         * 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.
@@ -792,8 +836,10 @@ public abstract class CronetEngine {
     /**
      * 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
@@ -801,21 +847,23 @@ public abstract class CronetEngine {
      * @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
@@ -843,7 +891,7 @@ public abstract class CronetEngine {
     /**
      * 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
@@ -970,8 +1018,7 @@ public abstract class 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}.