Enable NQE when Cronet Engine is built

Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it
possible to set NQE's delegate as socket performance watcher
factor in Http network session params. This in turn ensures
that NQE is notified of RTT observations from TCP and QUIC
sockets.

The previous API and the related tests will be removed after
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders. Those embedders
would continue to work.

BUG=618030
Committed: https://crrev.com/7018e2ad87f6f860e4d5fb42ceda5bd9998b664d
Cr-Commit-Position: refs/heads/master@{#402029}

[tbansal1, 2016-06-07 18:23:28]

Patchset #1 (id:1) has been deleted

[tbansal1, 2016-06-07 18:23:38]

Patchset #1 (id:20001) has been deleted

[tbansal1, 2016-06-07 18:37:35]

Description was changed from

==========
NQE

BUG=
==========

to

==========
Enable NQE as part of Cronet config

BUG=618030
==========

[tbansal1, 2016-06-07 18:47:58]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2016-06-07 20:02:04]

Patchset #1 (id:40001) has been deleted

[tbansal1, 2016-06-07 20:02:12]

Patchset #1 (id:80001) has been deleted

[tbansal1, 2016-06-07 20:02:20]

Patchset #1 (id:100001) has been deleted

[tbansal1, 2016-06-07 20:04:41]

Description was changed from

==========
Enable NQE as part of Cronet config

BUG=618030
==========

to

==========
Enable NQE as part of Cronet config

Enabling NQE as part of Cronet config makes it possible to
set socket performance watcher factor in Http network session
params. This in turn ensures that NQE is notified of RTT
observations from TCP and QUIC sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

BUG=618030
==========

[tbansal1, 2016-06-07 20:05:17]

tbansal@chromium.org changed reviewers:
+ bengr@chromium.org

[tbansal1, 2016-06-07 20:05:18]

bengr: ptal. Thanks.

[tbansal1, 2016-06-07 20:07:07]

Description was changed from

==========
Enable NQE as part of Cronet config

Enabling NQE as part of Cronet config makes it possible to
set socket performance watcher factor in Http network session
params. This in turn ensures that NQE is notified of RTT
observations from TCP and QUIC sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

BUG=618030
==========

to

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set socket performance watcher factor in Http 
network session params. This in turn ensures that NQE is 
notified of RTT observations from TCP and QUIC sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

BUG=618030
==========

[tbansal1, 2016-06-07 20:07:33]

Description was changed from

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set socket performance watcher factor in Http 
network session params. This in turn ensures that NQE is 
notified of RTT observations from TCP and QUIC sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

BUG=618030
==========

to

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

BUG=618030
==========

[bengr, 2016-06-07 20:53:17]

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:617: *
@param executor an executor that will be used to notified all
nit: notified -> notify

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:622: public
Builder enableNetworkQualityEstimator(boolean value, Executor executor) {
Why do consumers need to call this when they don't want NQE? I.e., wouldn't the
following suffice?

Builder enabledNetworkQualityEstimator(Executor executor)

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:625: if
(mNetworkQualityEstimatorEnabled && mNetworkQualityExecutor == null) {
With the new prototype, the first condition here should be removed.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:632: *
Returns if network quality estimator has been enabled for this builder.
Returns if -> Returns true if the

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:894: *
after network quality estimator has been enabled. Throughput observations
network -> the network

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1021: *
<p>This must be called after network quality estimator has been enabled.
after -> after the

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1030:
public abstract void addRequestFinishedListener(RequestFinishedListener
listener);
This should throw an exception if nqe is not enabled, right?

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:428:
DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
DCHECK also that the nqe is enabled.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:248:
if (!mNetworkQualityEstimatorEnabled) {
I was wondering if you even need this variable. E.g., you could have:

bool networkQualityEstimatorEnabled() {
  return mNetworkQualityExecutor != null;
}

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:255:
mCronetEngineBuilder.enableHTTP2(true).enableQUIC(false).enableNetworkQualityEstimator(
You shouldn't have to call enableNetworkQualityEstimator to *not* enable the
network quality estimator.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:74:
private final Object mLock = new Object();
Consider renaming as mObservationLock

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:228:
String quicURL = QuicTestServer.getServerURL() + "/simple.txt";
quicUrl? Shouldn't getServerURL be getServerUrl? I'm not clear on java style for
this.

https://codereview.chromium.org/2045703003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/2045703003/diff/120001/net/url_request/url_re...
net/url_request/url_request_context_builder.h:380:
SocketPerformanceWatcherFactory* socket_performance_watcher_factory_;
Should this be a WeakPtr? What ensures that this pointer is valid by the time
the builder builds?

[tbansal1, 2016-06-07 21:30:11]

Patchset #2 (id:140001) has been deleted

[tbansal1, 2016-06-07 21:32:35]

Patchset #2 (id:160001) has been deleted

[tbansal1, 2016-06-07 21:37:17]

bengr: ptal. Thanks!!

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:617: *
@param executor an executor that will be used to notified all
On 2016/06/07 20:53:16, bengr wrote:
> nit: notified -> notify

Done.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:622: public
Builder enableNetworkQualityEstimator(boolean value, Executor executor) {
On 2016/06/07 20:53:16, bengr wrote:
> Why do consumers need to call this when they don't want NQE? I.e., wouldn't
the
> following suffice?
> 
> Builder enabledNetworkQualityEstimator(Executor executor)

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.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:625: if
(mNetworkQualityEstimatorEnabled && mNetworkQualityExecutor == null) {
On 2016/06/07 20:53:16, bengr wrote:
> With the new prototype, the first condition here should be removed.

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

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:632: *
Returns if network quality estimator has been enabled for this builder.
On 2016/06/07 20:53:16, bengr wrote:
> Returns if -> Returns true if the

Done.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:894: *
after network quality estimator has been enabled. Throughput observations
On 2016/06/07 20:53:16, bengr wrote:
> network -> the network

Done.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1021: *
<p>This must be called after network quality estimator has been enabled.
On 2016/06/07 20:53:16, bengr wrote:
> after -> after the

Done.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1030:
public abstract void addRequestFinishedListener(RequestFinishedListener
listener);
On 2016/06/07 20:53:16, bengr wrote:
> This should throw an exception if nqe is not enabled, right?

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

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/cronet_url_request_context_adapter.cc (right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/cronet_url_request_context_adapter.cc:428:
DCHECK(GetNetworkTaskRunner()->BelongsToCurrentThread());
On 2016/06/07 20:53:16, bengr wrote:
> DCHECK also that the nqe is enabled.

The next line calls a function of network_quality_estimator_. If NQE is not
enabled that call will fail result in crash to nullptr. So, that's effectively
same as D-checking if NQE is enabled.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:248:
if (!mNetworkQualityEstimatorEnabled) {
On 2016/06/07 20:53:16, bengr wrote:
> I was wondering if you even need this variable. E.g., you could have:
> 
> bool networkQualityEstimatorEnabled() {
>   return mNetworkQualityExecutor != null;
> }

Done.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:255:
mCronetEngineBuilder.enableHTTP2(true).enableQUIC(false).enableNetworkQualityEstimator(
On 2016/06/07 20:53:16, bengr wrote:
> You shouldn't have to call enableNetworkQualityEstimator to *not* enable the
> network quality estimator.

Done.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:74:
private final Object mLock = new Object();
On 2016/06/07 20:53:16, bengr wrote:
> Consider renaming as mObservationLock

Done.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java:228:
String quicURL = QuicTestServer.getServerURL() + "/simple.txt";
On 2016/06/07 20:53:16, bengr wrote:
> quicUrl? Shouldn't getServerURL be getServerUrl? I'm not clear on java style
for
> this.

May be, may be not :) I am not sure about Java style either. But this is same as
Lines 123 and 149 above, so keeping it as such.

https://codereview.chromium.org/2045703003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/2045703003/diff/120001/net/url_request/url_re...
net/url_request/url_request_context_builder.h:380:
SocketPerformanceWatcherFactory* socket_performance_watcher_factory_;
On 2016/06/07 20:53:16, bengr wrote:
> Should this be a WeakPtr? What ensures that this pointer is valid by the time
> the builder builds?

It should not change during the lifetime of Builder or the URLRequestConfig
since it is owned by caller. Why do you think it will get destroyed?

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

lgtm, modulo a builder interface that I think cronet owners should weigh in on.

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:622: public
Builder enableNetworkQualityEstimator(boolean value, Executor executor) {
On 2016/06/07 21:37:16, tbansal1 wrote:
> On 2016/06/07 20:53:16, bengr wrote:
> > Why do consumers need to call this when they don't want NQE? I.e., wouldn't
> the
> > following suffice?
> > 
> > Builder enabledNetworkQualityEstimator(Executor executor)
> 
> 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.

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

https://codereview.chromium.org/2045703003/diff/120001/net/url_request/url_re...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/2045703003/diff/120001/net/url_request/url_re...
net/url_request/url_request_context_builder.h:380:
SocketPerformanceWatcherFactory* socket_performance_watcher_factory_;
On 2016/06/07 21:37:17, tbansal1 wrote:
> On 2016/06/07 20:53:16, bengr wrote:
> > Should this be a WeakPtr? What ensures that this pointer is valid by the
time
> > the builder builds?
> 
> It should not change during the lifetime of Builder or the URLRequestConfig
> since it is owned by caller. Why do you think it will get destroyed?

I guess this is ok.

[tbansal1, 2016-06-07 23:11:43]

tbansal@chromium.org changed reviewers:
+ mef@chromium.org

[tbansal1, 2016-06-07 23:11:44]

mef: ptal at components/cronet/. Thanks.

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

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2045703003/diff/120001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:622: public
Builder enableNetworkQualityEstimator(boolean value, Executor executor) {
On 2016/06/07 22:55:12, bengr wrote:
> On 2016/06/07 21:37:16, tbansal1 wrote:
> > On 2016/06/07 20:53:16, bengr wrote:
> > > Why do consumers need to call this when they don't want NQE? I.e.,
wouldn't
> > the
> > > following suffice?
> > > 
> > > Builder enabledNetworkQualityEstimator(Executor executor)
> > 
> > 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.
> 
> This seems like a really bad way to do things. Please check with a cronet
owner
> before landing this.

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.

[tbansal1, 2016-06-10 16:30:23]

Patchset #3 (id:200001) has been deleted

[tbansal1, 2016-06-13 16:13:40]

Patchset #3 (id:220001) has been deleted

[tbansal1, 2016-06-13 16:14:19]

Patchset #4 (id:260001) has been deleted

[tbansal1, 2016-06-13 16:14:28]

Patchset #3 (id:240001) has been deleted

[tbansal1, 2016-06-13 16:16:30]

Description was changed from

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

BUG=618030
==========

to

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders.

BUG=618030
==========

[tbansal1, 2016-06-13 16:16:57]

Description was changed from

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders.

BUG=618030
==========

to

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders. Those embedders
would continue to work.

BUG=618030
==========

[tbansal1, 2016-06-13 16:39:02]

Patchset #4 (id:300001) has been deleted

[tbansal1, 2016-06-13 16:40:47]

mef: PTAL. Thanks!!

[tbansal1, 2016-06-13 16:42:28]

On 2016/06/13 16:40:47, tbansal1 wrote:
> mef: PTAL. Thanks!!

Design doc linked in
https://bugs.chromium.org/p/chromium/issues/detail?id=618030#c3

[tbansal1, 2016-06-14 20:36:25]

mef: Ping . PTAL.

[mef, 2016-06-20 16:50:27]

Sorry for being so slow, it looks pretty good.

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:620: if
(!value) {
I think for Builder it is ok to be able to turn it on and off until actual
CronetEngine is built, so I would just store the value in
mNetworkQualityEstimatorEnabled and don't throw any exceptions.

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/NetworkQualityRttListener.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/NetworkQualityRttListener.java:33:
// all embedders start providing a non-null executor.
Wouldn't existing embedders use default constructor? 
Can we enable the check now?

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/NetworkQualityThroughputListener.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/NetworkQualityThroughputListener.java:24:
public NetworkQualityThroughputListener(Executor executor) {
Why does Throughput Listener require non-null executor and Rtt listener doesn't?

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:478:
Executor executor = listener.getExecutor() != null ? listener.getExecutor()
Why do we have this fallback in onRttObservation but not in
onThroughputObservation?

[tbansal1, 2016-06-20 20:06:57]

mef: ptal. Thanks.

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File components/cronet/android/api/src/org/chromium/net/CronetEngine.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/CronetEngine.java:620: if
(!value) {
On 2016/06/20 16:50:27, mef wrote:
> I think for Builder it is ok to be able to turn it on and off until actual
> CronetEngine is built, so I would just store the value in
> mNetworkQualityEstimatorEnabled and don't throw any exceptions.

Done.

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/NetworkQualityRttListener.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/NetworkQualityRttListener.java:33:
// all embedders start providing a non-null executor.
On 2016/06/20 16:50:27, mef wrote:
> Wouldn't existing embedders use default constructor? 
> Can we enable the check now?

Good point. Done. I had to modify the test code a little bit.

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File
components/cronet/android/api/src/org/chromium/net/NetworkQualityThroughputListener.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/api/src/org/chromium/net/NetworkQualityThroughputListener.java:24:
public NetworkQualityThroughputListener(Executor executor) {
On 2016/06/20 16:50:27, mef wrote:
> Why does Throughput Listener require non-null executor and Rtt listener
doesn't?

Because, the embedders are already using RttListener (so, I need to transition
that API in 2-steps).

Embedders are not using ThroughputListener yet, so I can change the API
abruptly.

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
File
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2045703003/diff/320001/components/cronet/andr...
components/cronet/android/java/src/org/chromium/net/CronetUrlRequestContext.java:478:
Executor executor = listener.getExecutor() != null ? listener.getExecutor()
On 2016/06/20 16:50:27, mef wrote:
> Why do we have this fallback in onRttObservation but not in
> onThroughputObservation?

Same reason as I commented elsewhere: "Because, the embedders are already using
RttListener (so, I need to transition
that API in 2-steps).

Embedders are not using ThroughputListener yet, so I can change the API
abruptly."

[tbansal1, 2016-06-22 13:11:22]

ping. It would be great if this is included in the next Cronet release :)

[tbansal1, 2016-06-23 14:00:26]

mef: friendly ping!

[mef, 2016-06-24 20:10:37]

lgtm, sorry for delay!

https://codereview.chromium.org/2045703003/diff/360001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2045703003/diff/360001/components/cronet/andr...
components/cronet/android/BUILD.gn:576:
"test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java",
You may also want to add those to cronet.gypi

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:626: bool
use_local_host_requests_for_tests) {
nit: use_local_host_requests_for_tests -> use_local_host_requests for
consistency.

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/throughput_ana...
File net/nqe/throughput_analyzer.h (right):

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/throughput_ana...
net/nqe/throughput_analyzer.h:77: bool use_local_host_requests_for_tests);
nit: drop '_for_tests' for consistency.

[tbansal1, 2016-06-24 22:41:39]

https://codereview.chromium.org/2045703003/diff/360001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2045703003/diff/360001/components/cronet/andr...
components/cronet/android/BUILD.gn:576:
"test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java",
On 2016/06/24 20:10:37, mef wrote:
> You may also want to add those to cronet.gypi

I am not sure where to add this in the cronet.gypi file. I do not see
QuicTest.java or SdchTest.java there (or any other file that is listed here).

Skipping it, also gyp is already deprecated for Android :).

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/network_qualit...
File net/nqe/network_quality_estimator.cc (right):

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/network_qualit...
net/nqe/network_quality_estimator.cc:626: bool
use_local_host_requests_for_tests) {
On 2016/06/24 20:10:37, mef wrote:
> nit: use_local_host_requests_for_tests -> use_local_host_requests for
> consistency.

Done.

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/throughput_ana...
File net/nqe/throughput_analyzer.h (right):

https://codereview.chromium.org/2045703003/diff/360001/net/nqe/throughput_ana...
net/nqe/throughput_analyzer.h:77: bool use_local_host_requests_for_tests);
On 2016/06/24 20:10:37, mef wrote:
> nit: drop '_for_tests' for consistency.

Done.

[mef, 2016-06-24 23:28:37]

still lgtm

https://codereview.chromium.org/2045703003/diff/360001/components/cronet/andr...
File components/cronet/android/BUILD.gn (right):

https://codereview.chromium.org/2045703003/diff/360001/components/cronet/andr...
components/cronet/android/BUILD.gn:576:
"test/javatests/src/org/chromium/net/TestNetworkQualityRttListener.java",
On 2016/06/24 22:41:39, tbansal1 wrote:
> On 2016/06/24 20:10:37, mef wrote:
> > You may also want to add those to cronet.gypi
> 
> I am not sure where to add this in the cronet.gypi file. I do not see
> QuicTest.java or SdchTest.java there (or any other file that is listed here)
Ah, cronet.gypi has 'java_in_dir': 'cronet/android/test/javatests', so it is
covered.
> 
> Skipping it, also gyp is already deprecated for Android :).
Good point, we need to remove that.

[tbansal1, 2016-06-25 00:05:51]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-06-25 00:05:51]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2045703003/#ps400001
(title: "Addressed mef comments")

[commit-bot: I haz the power, 2016-06-25 00:06:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-06-25 00:40:54]

Description was changed from

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders. Those embedders
would continue to work.

BUG=618030
==========

to

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders. Those embedders
would continue to work.

BUG=618030
==========

[commit-bot: I haz the power, 2016-06-25 00:40:55]

Committed patchset #8 (id:400001)

[commit-bot: I haz the power, 2016-06-25 00:42:34]

Description was changed from

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it 
possible to set NQE's delegate as socket performance watcher 
factor in Http network session params. This in turn ensures 
that NQE is notified of RTT observations from TCP and QUIC 
sockets.

The previous API and the related tests will be removed after 
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders. Those embedders
would continue to work.

BUG=618030
==========

to

==========
Enable NQE when Cronet Engine is built

Enabling NQE as soon as Cronet Engine is built makes it
possible to set NQE's delegate as socket performance watcher
factor in Http network session params. This in turn ensures
that NQE is notified of RTT observations from TCP and QUIC
sockets.

The previous API and the related tests will be removed after
the existing consumers of that API transfer to using Cronet
config for enabling NQE.

Also, update the network quality listeners API. Now, RTT
and throughput listeners are required to provide an
executor in their constructor. The listeners are then
provided the observations on the same executor.

This CL does not modify UrlRequestFinishedListener, which
is already in use by multiple embedders. Those embedders
would continue to work.

BUG=618030

Committed: https://crrev.com/7018e2ad87f6f860e4d5fb42ceda5bd9998b664d
Cr-Commit-Position: refs/heads/master@{#402029}
==========

[commit-bot: I haz the power, 2016-06-25 00:42:35]

Patchset 8 (id:??) landed as
https://crrev.com/7018e2ad87f6f860e4d5fb42ceda5bd9998b664d
Cr-Commit-Position: refs/heads/master@{#402029}