Expose effective connection type to Cronet

Expose effective connection type to Cronet

CronetURLRequestContextAdapter registers as an Effective
Connection Type (ECT) Observer. When it receives
notifications on network thread about changes in ECT, it
passes them to Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink
bandwidth. This CL also adds default transport RTT
thresholds for different effective connection types so
that NQE can compute ECT when thresholds have not been
specified using field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305
Committed: https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf
Cr-Commit-Position: refs/heads/master@{#411462}

[tbansal1, 2016-08-08 16:02:49]

Description was changed from

==========
Expose effective connection type to Cronet

CronetURLContextAdapter registers as an Effective Connection
Type (ECT) Observer. When it receives notifications on
network thread about changes in ECT, it passes them to
Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink bandwidth.
This CL also adds default transport RTT thresholds for
different effective connection types so that NQE can
compute ECT when thresholds have not been specified using
field trial params (as is the case for Cronet).

BUG=625305
==========

to

==========
Expose effective connection type to Cronet

CronetURLContextAdapter registers as an Effective Connection
Type (ECT) Observer. When it receives notifications on
network thread about changes in ECT, it passes them to
Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink bandwidth.
This CL also adds default transport RTT thresholds for
different effective connection types so that NQE can
compute ECT when thresholds have not been specified using
field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305
==========

[tbansal1, 2016-08-08 16:03:42]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 16:04:07]

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

[tbansal1, 2016-08-08 16:32:18]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-08 16:32:42]

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

[tbansal1, 2016-08-08 16:48:18]

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

[tbansal1, 2016-08-08 16:48:19]

xunjieli: PTAL at diff of PS #1 and PS #2.
Thanks.

[commit-bot: I haz the power, 2016-08-08 17:28:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-08 17:28:18]

Dry run: This issue passed the CQ dry run.

[xunjieli, 2016-08-09 13:22:44]

Looks good! One last request, could you build cronet_package, and check in the
cronet/JavaDoc folder in the output directory to see that
EffectiveConnectionType.java is there?

https://codereview.chromium.org/2223773003/diff/20001/components/cronet/andro...
File
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
(right):

https://codereview.chromium.org/2223773003/diff/20001/components/cronet/andro...
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:86:
private int mEffectiveConnectionType =
I think this should be:

@EffectiveConnectionType
private int mEffectiveConnectionType =

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
File net/nqe/effective_connection_type.h (right):

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
net/nqe/effective_connection_type.h:22: // GENERATED_JAVA_CLASS_NAME_OVERRIDE:
EffectiveConnectionType
You can omit this line 22.

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
net/nqe/effective_connection_type.h:23: // GENERATED_JAVA_PREFIX_TO_STRIP:
PREFIX_NOT_PRESENT_
Do you need to keep the prefix? If not, can we strip the
EFFECTIVE_CONNECTION_TYPE_?

[tbansal1, 2016-08-09 17:54:23]

On 2016/08/09 13:22:44, xunjieli wrote:
> Looks good! One last request, could you build cronet_package, and check in the
> cronet/JavaDoc folder in the output directory to see that
> EffectiveConnectionType.java is there?
I do not see the documentation there. But, then I do not see documentation for
UrlRequestError either. Also, I am not sure how this exactly works because
url_request_error.h has an explicitly added .java file that maps the variables
to Java-land. So, may be we should also be doing something similar (that would
be similar to PS 1). wdyt?
> 
>
https://codereview.chromium.org/2223773003/diff/20001/components/cronet/andro...
> File
>
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
> (right):
> 
>
https://codereview.chromium.org/2223773003/diff/20001/components/cronet/andro...
>
components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:86:
> private int mEffectiveConnectionType =
> I think this should be:
> 
> @EffectiveConnectionType
> private int mEffectiveConnectionType =
> 
>
https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
> File net/nqe/effective_connection_type.h (right):
> 
>
https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
> net/nqe/effective_connection_type.h:22: // GENERATED_JAVA_CLASS_NAME_OVERRIDE:
> EffectiveConnectionType
> You can omit this line 22.
> 
>
https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
> net/nqe/effective_connection_type.h:23: // GENERATED_JAVA_PREFIX_TO_STRIP:
> PREFIX_NOT_PRESENT_
> Do you need to keep the prefix? If not, can we strip the
> EFFECTIVE_CONNECTION_TYPE_?

[tbansal1, 2016-08-09 17:59:16]

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
File net/nqe/effective_connection_type.h (right):

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
net/nqe/effective_connection_type.h:22: // GENERATED_JAVA_CLASS_NAME_OVERRIDE:
EffectiveConnectionType
On 2016/08/09 13:22:44, xunjieli wrote:
> You can omit this line 22.

Done.

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
net/nqe/effective_connection_type.h:23: // GENERATED_JAVA_PREFIX_TO_STRIP:
PREFIX_NOT_PRESENT_
On 2016/08/09 13:22:44, xunjieli wrote:
> Do you need to keep the prefix? If not, can we strip the
> EFFECTIVE_CONNECTION_TYPE_?

If I remove this, then EFFECTIVE_CONNECTION_TYPE_ is automatically stripped, and
the variable names in the java file reduce to
UNKNOWN
OFFLINE
SLOW_2G
2G // This fails to compile, and it is also against conventions.
...

[xunjieli, 2016-08-09 17:59:47]

On 2016/08/09 17:54:23, tbansal1 wrote:
> On 2016/08/09 13:22:44, xunjieli wrote:
> > Looks good! One last request, could you build cronet_package, and check in
the
> > cronet/JavaDoc folder in the output directory to see that
> > EffectiveConnectionType.java is there?
> I do not see the documentation there. But, then I do not see documentation for
> UrlRequestError either. Also, I am not sure how this exactly works because
> url_request_error.h has an explicitly added .java file that maps the variables
> to Java-land. So, may be we should also be doing something similar (that would
> be similar to PS 1). wdyt?

No, I prefer not adding an explicit .java file, since we already have a
generated java file -- that will make us increase our jar files for no reason.
I will take a look later today.

[xunjieli, 2016-08-10 17:39:43]

There is a typo in the Cl description. CronetURLContextAdapter ->
CronetURLRequestContextAdapter.

The jar source part takes a bit more effort to make it work. I will put it in my
queue. 

LGTM once the following comment is addressed.

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
File net/nqe/effective_connection_type.h (right):

https://codereview.chromium.org/2223773003/diff/20001/net/nqe/effective_conne...
net/nqe/effective_connection_type.h:23: // GENERATED_JAVA_PREFIX_TO_STRIP:
PREFIX_NOT_PRESENT_
On 2016/08/09 17:59:16, tbansal1 wrote:
> On 2016/08/09 13:22:44, xunjieli wrote:
> > Do you need to keep the prefix? If not, can we strip the
> > EFFECTIVE_CONNECTION_TYPE_?
> 
> If I remove this, then EFFECTIVE_CONNECTION_TYPE_ is automatically stripped,
and
> the variable names in the java file reduce to
> UNKNOWN
> OFFLINE
> SLOW_2G
> 2G // This fails to compile, and it is also against conventions.
> ...

Acknowledged.

https://codereview.chromium.org/2223773003/diff/40001/net/nqe/effective_conne...
File net/nqe/effective_connection_type.h (right):

https://codereview.chromium.org/2223773003/diff/40001/net/nqe/effective_conne...
net/nqe/effective_connection_type.h:32: EFFECTIVE_CONNECTION_TYPE_LAST,
Could you move over the comments from PS1 to this header file? That way we will
get them on the Java side too.

[xunjieli, 2016-08-11 15:13:38]

FYI: my follow-up CL is at https://codereview.chromium.org/2234113002/.

[tbansal1, 2016-08-11 17:35:38]

Patchset #4 (id:60001) has been deleted

[tbansal1, 2016-08-11 17:38:26]

Description was changed from

==========
Expose effective connection type to Cronet

CronetURLContextAdapter registers as an Effective Connection
Type (ECT) Observer. When it receives notifications on
network thread about changes in ECT, it passes them to
Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink bandwidth.
This CL also adds default transport RTT thresholds for
different effective connection types so that NQE can
compute ECT when thresholds have not been specified using
field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305
==========

to

==========
Expose effective connection type to Cronet

CronetURLRequestContextAdapter registers as an Effective
Connection Type (ECT) Observer. When it receives 
notifications on network thread about changes in ECT, it 
passes them to Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink 
bandwidth. This CL also adds default transport RTT 
thresholds for different effective connection types so 
that NQE can compute ECT when thresholds have not been 
specified using field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305
==========

[tbansal1, 2016-08-11 17:42:33]

Patchset #4 (id:80001) has been deleted

[tbansal1, 2016-08-11 17:43:12]

https://codereview.chromium.org/2223773003/diff/40001/net/nqe/effective_conne...
File net/nqe/effective_connection_type.h (right):

https://codereview.chromium.org/2223773003/diff/40001/net/nqe/effective_conne...
net/nqe/effective_connection_type.h:32: EFFECTIVE_CONNECTION_TYPE_LAST,
On 2016/08/10 17:39:43, xunjieli wrote:
> Could you move over the comments from PS1 to this header file? That way we
will
> get them on the Java side too.

Done.

[tbansal1, 2016-08-11 17:43:19]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 17:44:02]

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

[xunjieli, 2016-08-11 17:45:27]

lgtm

https://codereview.chromium.org/2223773003/diff/100001/net/nqe/effective_conn...
File net/nqe/effective_connection_type.h (right):

https://codereview.chromium.org/2223773003/diff/100001/net/nqe/effective_conn...
net/nqe/effective_connection_type.h:24: // The connection types should be in
increasing order of quality.
Can you move this line above enum EffectiveConnectionType?
Otherwis, the generated Java file will have this line in the JavaDoc for the
first enum.

[commit-bot: I haz the power, 2016-08-11 17:46:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 17:46:56]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[tbansal1, 2016-08-11 18:00:33]

Patchset #4 (id:100001) has been deleted

[tbansal1, 2016-08-11 18:00:53]

Patchset #4 (id:120001) has been deleted

[tbansal1, 2016-08-11 18:01:15]

The CQ bit was checked by tbansal@chromium.org to run a CQ dry run

[tbansal1, 2016-08-11 18:01:43]

xunjieli: I addressed the comment (and I deleted that PS by mistake).

[commit-bot: I haz the power, 2016-08-11 18:02:16]

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

[xunjieli, 2016-08-11 18:46:42]

On 2016/08/11 18:01:43, tbansal1 wrote:
> xunjieli: I addressed the comment (and I deleted that PS by mistake).

lgtm

[commit-bot: I haz the power, 2016-08-11 19:31:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 19:31:01]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[tbansal1, 2016-08-11 22:48:21]

The CQ bit was checked by tbansal@chromium.org

[commit-bot: I haz the power, 2016-08-11 22:49:28]

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-08-12 00:07:56]

Description was changed from

==========
Expose effective connection type to Cronet

CronetURLRequestContextAdapter registers as an Effective
Connection Type (ECT) Observer. When it receives 
notifications on network thread about changes in ECT, it 
passes them to Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink 
bandwidth. This CL also adds default transport RTT 
thresholds for different effective connection types so 
that NQE can compute ECT when thresholds have not been 
specified using field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305
==========

to

==========
Expose effective connection type to Cronet

CronetURLRequestContextAdapter registers as an Effective
Connection Type (ECT) Observer. When it receives 
notifications on network thread about changes in ECT, it 
passes them to Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink 
bandwidth. This CL also adds default transport RTT 
thresholds for different effective connection types so 
that NQE can compute ECT when thresholds have not been 
specified using field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305
==========

[commit-bot: I haz the power, 2016-08-12 00:07:58]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-08-12 00:11:23]

Description was changed from

==========
Expose effective connection type to Cronet

CronetURLRequestContextAdapter registers as an Effective
Connection Type (ECT) Observer. When it receives 
notifications on network thread about changes in ECT, it 
passes them to Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink 
bandwidth. This CL also adds default transport RTT 
thresholds for different effective connection types so 
that NQE can compute ECT when thresholds have not been 
specified using field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305
==========

to

==========
Expose effective connection type to Cronet

CronetURLRequestContextAdapter registers as an Effective
Connection Type (ECT) Observer. When it receives
notifications on network thread about changes in ECT, it
passes them to Java land where ECT is stored in a guarded variable.

ECT is computed using transport RTT and downlink
bandwidth. This CL also adds default transport RTT
thresholds for different effective connection types so
that NQE can compute ECT when thresholds have not been
specified using field trial params (as is the case for Cronet).

This is reland of https://codereview.chromium.org/2146563002/

BUG=625305

Committed: https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf
Cr-Commit-Position: refs/heads/master@{#411462}
==========

[commit-bot: I haz the power, 2016-08-12 00:11:24]

Patchset 4 (id:??) landed as
https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf
Cr-Commit-Position: refs/heads/master@{#411462}