New issue
Advanced search Search tips

Issue 625305 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

NQE should expose GetEffectiveConnectionType API to Cronet

Project Member Reported by tbansal@chromium.org, Jul 1 2016

Issue description

NQE should expose GetEffectiveConnectionType API to Cronet so that Cronet embedders can use it instead of duplicating the modeling/estimating component of NQE.
 
Project Member

Comment 2 by bugdroid1@chromium.org, Jul 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2739868cb29af2c9978389c38c437899dd827208

commit 2739868cb29af2c9978389c38c437899dd827208
Author: tbansal <tbansal@chromium.org>
Date: Wed Jul 27 00:44:20 2016

Provide default thresholds for effective connection types

Provide default HTTP RTT and transport RTT thresholds for
Slow2G and 2G effective connection types. The default
thresholds can still be overriden using field trial
variation params.

Advantage of this approach is that we would no longer need
to specify the variation params in the field trial configs
(keeps the configs cleaner). Also, it enables net stack
embedders such as Cronet that do not have the concept of
field trial to also compute ECT because the thresholds
would be available in the code itself.

BUG= 625305 

Review-Url: https://codereview.chromium.org/2172403002
Cr-Commit-Position: refs/heads/master@{#407993}

[modify] https://crrev.com/2739868cb29af2c9978389c38c437899dd827208/net/nqe/network_quality.cc
[modify] https://crrev.com/2739868cb29af2c9978389c38c437899dd827208/net/nqe/network_quality.h
[modify] https://crrev.com/2739868cb29af2c9978389c38c437899dd827208/net/nqe/network_quality_estimator.cc
[modify] https://crrev.com/2739868cb29af2c9978389c38c437899dd827208/net/nqe/network_quality_estimator.h
[modify] https://crrev.com/2739868cb29af2c9978389c38c437899dd827208/net/nqe/network_quality_estimator_unittest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 3 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16cad789693aa8032dec86a8585e0dcec5f6124e

commit 16cad789693aa8032dec86a8585e0dcec5f6124e
Author: tbansal <tbansal@chromium.org>
Date: Wed Aug 03 21:18:29 2016

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 

Review-Url: https://codereview.chromium.org/2146563002
Cr-Commit-Position: refs/heads/master@{#409624}

[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/BUILD.gn
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[add] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/cronet_url_request_context_adapter.h
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/net/nqe/effective_connection_type.h
[modify] https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e/net/nqe/network_quality_estimator.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Aug 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/aa64633ecf68180fe381a0b5c31b874d4aab7ee8

commit aa64633ecf68180fe381a0b5c31b874d4aab7ee8
Author: tbansal <tbansal@chromium.org>
Date: Thu Aug 04 19:47:32 2016

Revert of Expose effective connection type to Cronet (patchset #6 id:200001 of https://codereview.chromium.org/2146563002/ )

Reason for revert:
Speculative revert to see if this fixes failing tests on ClangToTWin(dbg) tester:
https://build.chromium.org/p/chromium.fyi/builders/ClangToTWin%28dbg%29%20tester/builds/2913

Original issue's description:
> 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 
>
> Committed: https://crrev.com/16cad789693aa8032dec86a8585e0dcec5f6124e
> Cr-Commit-Position: refs/heads/master@{#409624}

TBR=ryansturm@chromium.org,xunjieli@chromium.org,thakis@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 625305 

Review-Url: https://codereview.chromium.org/2214163002
Cr-Commit-Position: refs/heads/master@{#409873}

[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/BUILD.gn
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[delete] https://crrev.com/01a32f17f8bf3ddf8c2a523ea30a44ba21418453/components/cronet/android/api/src/org/chromium/net/EffectiveConnectionType.java
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/cronet_url_request_context_adapter.h
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/net/nqe/effective_connection_type.h
[modify] https://crrev.com/aa64633ecf68180fe381a0b5c31b874d4aab7ee8/net/nqe/network_quality_estimator.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Aug 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/465fad09d77199ccee02ccf304fb8c5bd92d94cf

commit 465fad09d77199ccee02ccf304fb8c5bd92d94cf
Author: tbansal <tbansal@chromium.org>
Date: Fri Aug 12 00:06:56 2016

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 

Review-Url: https://codereview.chromium.org/2223773003
Cr-Commit-Position: refs/heads/master@{#411462}

[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/BUILD.gn
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/api/src/org/chromium/net/JavaCronetEngine.java
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/cronet_url_request_context_adapter.cc
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/cronet_url_request_context_adapter.h
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/components/cronet/android/test/javatests/src/org/chromium/net/QuicTest.java
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/net/nqe/effective_connection_type.h
[modify] https://crrev.com/465fad09d77199ccee02ccf304fb8c5bd92d94cf/net/nqe/network_quality_estimator.cc

Status: Fixed (was: Started)
Labels: NQE
Components: Internals>Network>NetworkQuality
Labels: -nqe

Sign in to add a comment