Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2417643007:
Expose RTT and throughput estimates from Cronet (Closed)
DescriptionExpose RTT and throughput estimates from Cronet
The estimates are computed by the network quality estimator
(NQE) and are pushed to CronetUrlRequestContext.
Embedders can query for the estimates by calling
appropriate methods on CronetEngine.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet
BUG=654498
Committed: https://crrev.com/dbd3adb57de9b1802723ddb1f27081ffd87eba73
Cr-Commit-Position: refs/heads/master@{#426517}
Patch Set 1 : ps #
Total comments: 20
Patch Set 2 : Addressed mgersh comments #Patch Set 3 : Fix the java doc #
Total comments: 6
Patch Set 4 : Addressed xunjieli comments #
Total comments: 4
Patch Set 5 : fix javadoc #
Total comments: 2
Patch Set 6 : Addressed comments #
Total comments: 2
Patch Set 7 : Removed annotation #Messages
Total messages: 84 (55 generated)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Your CL can not be processed by CQ because of: * Failed to parse additional trybots
Description was changed from ========== Do not submit CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester:android_cronet BUG= ========== to ========== Do not submit CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG= ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Patchset #1 (id:20001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:100001) has been deleted
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
Description was changed from ========== Do not submit CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG= ========== to ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. BUG= ==========
tbansal@chromium.org changed reviewers: + mgersh@chromium.org, ryansturm@chromium.org
ryan: ptal at //net/ or everything. mgersh: ptal at //components/cronet. Thanks.
Description was changed from ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. BUG= ========== to ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. BUG=654498 ==========
Description was changed from ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. BUG=654498 ========== to ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=654498 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:365: synchronized (mLock) { I'm not happy with the locking order pattern in this file. Could synchronized (mLock) { checkHaveAdapter(); } happen before synchronized (mNetworkQualityLock) instead of inside it? https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:15: import org.json.JSONObject; Should this import be alphabetical (i.e., after java.io.FileReader) or is there some limitation? https://codereview.chromium.org/2417643007/diff/80001/net/nqe/network_quality.h File net/nqe/network_quality.h (right): https://codereview.chromium.org/2417643007/diff/80001/net/nqe/network_quality... net/nqe/network_quality.h:38: const int32_t kInvalidThroughput = INVALID_RTT_THROUGHPUT_VALUE; Why was this exposed in the header as a const? Seems like this should be an extern const or a macro. If you're removing it later that this is fine to leave as is.
ptal. thanks. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:365: synchronized (mLock) { On 2016/10/14 21:21:32, Ryan Sturm wrote: > I'm not happy with the locking order pattern in this file. Could > > synchronized (mLock) { > checkHaveAdapter(); > } > > happen before > > synchronized (mNetworkQualityLock) > > instead of inside it? This pattern was introduced in https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr.... Not sure why it is this way. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:15: import org.json.JSONObject; On 2016/10/14 21:21:32, Ryan Sturm wrote: > Should this import be alphabetical (i.e., after java.io.FileReader) or is there > some limitation? This is the new alphabetical order. See https://codereview.chromium.org/2052823002. https://codereview.chromium.org/2417643007/diff/80001/net/nqe/network_quality.h File net/nqe/network_quality.h (right): https://codereview.chromium.org/2417643007/diff/80001/net/nqe/network_quality... net/nqe/network_quality.h:38: const int32_t kInvalidThroughput = INVALID_RTT_THROUGHPUT_VALUE; On 2016/10/14 21:21:32, Ryan Sturm wrote: > Why was this exposed in the header as a const? Seems like this should be an > extern const or a macro. If you're removing it later that this is fine to leave > as is. Yup, this is going to be removed soon.
https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:365: synchronized (mLock) { On 2016/10/14 21:50:29, tbansal1 wrote: > On 2016/10/14 21:21:32, Ryan Sturm wrote: > > I'm not happy with the locking order pattern in this file. Could > > > > synchronized (mLock) { > > checkHaveAdapter(); > > } > > > > happen before > > > > synchronized (mNetworkQualityLock) > > > > instead of inside it? > > This pattern was introduced in > https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr.... > Not sure why it is this way. Any chance you want to try to fix it in this CL? There doesn't seem to be a need to have nested locks, and I would hate to block the IO thread if mLock is stuck doing something. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:15: import org.json.JSONObject; On 2016/10/14 21:50:29, tbansal1 wrote: > On 2016/10/14 21:21:32, Ryan Sturm wrote: > > Should this import be alphabetical (i.e., after java.io.FileReader) or is > there > > some limitation? > > This is the new alphabetical order. See > https://codereview.chromium.org/2052823002. Acknowledged. This breaks my brain a little, but I guess it's the same as chromium c++ include order having 3 distinct groupings, except this is 10 groupings. https://codereview.chromium.org/2417643007/diff/80001/net/nqe/network_quality.h File net/nqe/network_quality.h (right): https://codereview.chromium.org/2417643007/diff/80001/net/nqe/network_quality... net/nqe/network_quality.h:38: const int32_t kInvalidThroughput = INVALID_RTT_THROUGHPUT_VALUE; On 2016/10/14 21:50:29, tbansal1 wrote: > On 2016/10/14 21:21:32, Ryan Sturm wrote: > > Why was this exposed in the header as a const? Seems like this should be an > > extern const or a macro. If you're removing it later that this is fine to > leave > > as is. > > Yup, this is going to be removed soon. Acknowledged.
https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:365: synchronized (mLock) { On 2016/10/14 22:19:12, Ryan Sturm wrote: > On 2016/10/14 21:50:29, tbansal1 wrote: > > On 2016/10/14 21:21:32, Ryan Sturm wrote: > > > I'm not happy with the locking order pattern in this file. Could > > > > > > synchronized (mLock) { > > > checkHaveAdapter(); > > > } > > > > > > happen before > > > > > > synchronized (mNetworkQualityLock) > > > > > > instead of inside it? > > > > This pattern was introduced in > > > https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr.... > > Not sure why it is this way. > > Any chance you want to try to fix it in this CL? If we do want to fix it, I would prefer to do it in a separate CL. > There doesn't seem to be a need > to have nested locks, and I would hate to block the IO thread if mLock is stuck > doing something. I am not sure how separating the two locks makes it faster. If mLock is stuck, then even if the locks are separated, NQE functions still have to wait.
https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:365: synchronized (mLock) { On 2016/10/14 22:44:43, tbansal1 wrote: > On 2016/10/14 22:19:12, Ryan Sturm wrote: > > On 2016/10/14 21:50:29, tbansal1 wrote: > > > On 2016/10/14 21:21:32, Ryan Sturm wrote: > > > > I'm not happy with the locking order pattern in this file. Could > > > > > > > > synchronized (mLock) { > > > > checkHaveAdapter(); > > > > } > > > > > > > > happen before > > > > > > > > synchronized (mNetworkQualityLock) > > > > > > > > instead of inside it? > > > > > > This pattern was introduced in > > > > > > https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr.... > > > Not sure why it is this way. > > > > Any chance you want to try to fix it in this CL? > If we do want to fix it, I would prefer to do it in a separate CL. > > There doesn't seem to be a need > > to have nested locks, and I would hate to block the IO thread if mLock is > stuck > > doing something. > I am not sure how separating the two locks makes it faster. If mLock is stuck, > then > even if the locks are separated, NQE functions still have to wait. If getEffectiveConnectionTypeChanged gets blocked on mLock on the UI thread, then calling onEffectiveConnectionTypeChanged will block the IO thread, but if mNetworkQualityLock weren't held then onEffectiveConnectionTypeChanged could just set it and leave. Change it in another CL. Doesn't seem high priority.
https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:365: synchronized (mLock) { On 2016/10/14 22:55:38, Ryan Sturm wrote: > On 2016/10/14 22:44:43, tbansal1 wrote: > > On 2016/10/14 22:19:12, Ryan Sturm wrote: > > > On 2016/10/14 21:50:29, tbansal1 wrote: > > > > On 2016/10/14 21:21:32, Ryan Sturm wrote: > > > > > I'm not happy with the locking order pattern in this file. Could > > > > > > > > > > synchronized (mLock) { > > > > > checkHaveAdapter(); > > > > > } > > > > > > > > > > happen before > > > > > > > > > > synchronized (mNetworkQualityLock) > > > > > > > > > > instead of inside it? > > > > > > > > This pattern was introduced in > > > > > > > > > > https://codereview.chromium.org/2283243002/diff/120001/components/cronet/andr.... > > > > Not sure why it is this way. > > > > > > Any chance you want to try to fix it in this CL? > > If we do want to fix it, I would prefer to do it in a separate CL. > > > There doesn't seem to be a need > > > to have nested locks, and I would hate to block the IO thread if mLock is > > stuck > > > doing something. > > I am not sure how separating the two locks makes it faster. If mLock is stuck, > > then > > even if the locks are separated, NQE functions still have to wait. > > If getEffectiveConnectionTypeChanged gets blocked on mLock on the UI thread, > then calling onEffectiveConnectionTypeChanged will block the IO thread, but if > mNetworkQualityLock weren't held then onEffectiveConnectionTypeChanged could > just set it and leave. > > Change it in another CL. Doesn't seem high priority. Yeah, I need to check with clm@ why it was done this way.
mgersh@chromium.org changed reviewers: + xunjieli@chromium.org
lgtm with a few comments. Adding Helen to also look at it since I'm not actually in OWNERS yet. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:785: * a valid value is unavailable, nit: comma should be period https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1003: * quality estimator. Set to {@link INVALID_RTT_THROUGHPUT} if a valid value This javadoc and the others could mention that you need to enable NQE before using it. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1008: public abstract int getHttpRttMsec(); Elsewhere in Cronet we use Ms, not Msec. Let's change this to Ms for consistency. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1017: public abstract int getTransportRttMsec(); same as above: Msec -> Ms
lgtm if you figure out the locking (add a comment to all the methods about why the locking order is that way if do leave it as is)
Patchset #2 (id:120001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/10/17 18:21:07, Ryan Sturm wrote: > lgtm if you figure out the locking (add a comment to all the methods about why > the locking order is that way if do leave it as is) Going to file a bug soon.
xunjieli: ptal at *cronet* Thanks. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:785: * a valid value is unavailable, On 2016/10/17 16:53:09, mgersh wrote: > nit: comma should be period Done. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1003: * quality estimator. Set to {@link INVALID_RTT_THROUGHPUT} if a valid value On 2016/10/17 16:53:09, mgersh wrote: > This javadoc and the others could mention that you need to enable NQE before > using it. Done. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1008: public abstract int getHttpRttMsec(); On 2016/10/17 16:53:09, mgersh wrote: > Elsewhere in Cronet we use Ms, not Msec. Let's change this to Ms for > consistency. Done. https://codereview.chromium.org/2417643007/diff/80001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:1017: public abstract int getTransportRttMsec(); On 2016/10/17 16:53:09, mgersh wrote: > same as above: Msec -> Ms Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
My concern is that there are three APIs added on CronetEngine level that are only applicable when NQE is enabled. Could you write a brief design doc and send it out to cronet@ ? Others have put in a lot of effort in making the API GMSCore-compatible, so I think any major API change now should at least have a design review to make sure everyone is fine with it.
On 2016/10/17 21:27:27, xunjieli wrote: > My concern is that there are three APIs added on CronetEngine level that are > only applicable when NQE is enabled. > Could you write a brief design doc and send it out to cronet@ ? > Others have put in a lot of effort in making the API GMSCore-compatible, so I > think any major API change now should at least have a design review to make sure > everyone is fine with it. I will write something today. Can you elaborate on what makes an API GMSCore-compatible? Is there something I can do I this CL to make it easier to port? Thanks.
On 2016/10/17 21:59:04, tbansal1 wrote: > On 2016/10/17 21:27:27, xunjieli wrote: > > My concern is that there are three APIs added on CronetEngine level that are > > only applicable when NQE is enabled. > > Could you write a brief design doc and send it out to cronet@ ? > > Others have put in a lot of effort in making the API GMSCore-compatible, so I > > think any major API change now should at least have a design review to make > sure > > everyone is fine with it. > > I will write something today. Can you elaborate on what makes an API > GMSCore-compatible? > Is there something I can do I this CL to make it easier to port? > Thanks. Also, I am not sure there is much to say in a design doc. The logic for all 3 APIs is exactly same as the existing getEffectiveConnectionType() API. Is there something else that I should mention in the design doc?
On 2016/10/17 21:59:04, tbansal1 wrote: > On 2016/10/17 21:27:27, xunjieli wrote: > > My concern is that there are three APIs added on CronetEngine level that are > > only applicable when NQE is enabled. > > Could you write a brief design doc and send it out to cronet@ ? > > Others have put in a lot of effort in making the API GMSCore-compatible, so I > > think any major API change now should at least have a design review to make > sure > > everyone is fine with it. > > I will write something today. Can you elaborate on what makes an API > GMSCore-compatible? > Is there something I can do I this CL to make it easier to port? > Thanks. Paul and Andrei would know the best. They did a lot of work to make sure there is a well defined split between implementation and API, and all the APIs adhere the best practices and guidelines. I do not know the details. I think this change is probably fine, but it's good to double check (by doing a short design review), so there won't be surprises once this lands.
On 2016/10/17 22:05:50, xunjieli wrote: > On 2016/10/17 21:59:04, tbansal1 wrote: > > On 2016/10/17 21:27:27, xunjieli wrote: > > > My concern is that there are three APIs added on CronetEngine level that are > > > only applicable when NQE is enabled. > > > Could you write a brief design doc and send it out to cronet@ ? > > > Others have put in a lot of effort in making the API GMSCore-compatible, so > I > > > think any major API change now should at least have a design review to make > > sure > > > everyone is fine with it. > > > > I will write something today. Can you elaborate on what makes an API > > GMSCore-compatible? > > Is there something I can do I this CL to make it easier to port? > > Thanks. > > Paul and Andrei would know the best. They did a lot of work to make sure there > is a well defined split between implementation and API, and all the APIs adhere > the best practices and guidelines. I do not know the details. I think this > change is probably fine, but it's good to double check (by doing a short design > review), so there won't be surprises once this lands. Instead of a design doc, you could also summarize everything in an email. I suggested design doc so it's easier to add comments and do follow-ups on the comments.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:786: public static final int INVALID_RTT_THROUGHPUT = -1; nit: Reuse the one that you generated in RttThroughputValues.java https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:366: checkHaveAdapter(); Why do we need to checkHaveAdapter() here? Can we just return the result directly? If the adapter isn't initialized or torn down, we will return invalid, which seems reasonable.
xunjieli: ptal. thanks. https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:786: public static final int INVALID_RTT_THROUGHPUT = -1; On 2016/10/18 21:52:00, xunjieli wrote: > nit: Reuse the one that you generated in RttThroughputValues.java Done. https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:366: checkHaveAdapter(); On 2016/10/18 21:52:00, xunjieli wrote: > Why do we need to checkHaveAdapter() here? Can we just return the result > directly? If the adapter isn't initialized or torn down, we will return invalid, > which seems reasonable. I suppose checking the adapter helps in catching errors. Just like it is checked in other methods.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:366: checkHaveAdapter(); On 2016/10/19 21:51:39, tbansal1 wrote: > On 2016/10/18 21:52:00, xunjieli wrote: > > Why do we need to checkHaveAdapter() here? Can we just return the result > > directly? If the adapter isn't initialized or torn down, we will return > invalid, > > which seems reasonable. > > I suppose checking the adapter helps in catching errors. Just like it is checked > in other methods. In other places (except getEffectiveConnectionType()), checkHaveAdapter() makes sure we are not calling into native code with a null object. Since we are not calling into native code, there isn't a reason to incur the synchonization cost. I'd suggest removing synchronized(mLock) { checkHaveAdapter(); } https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:96: private int mEffectiveConnectionType = EffectiveConnectionType.TYPE_UNKNOWN; Since you are here, could you add a @EffectiveConnectionType above this line? @EffectiveConnectionType private int mEffectiveConnectionType = EffectiveConnectionType.TYPE_UNKNOWN; I believe it's legal coz we now have IntDef support in java_cpp_enum.py https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:284: assert CronetEngine.INVALID_RTT_THROUGHPUT < 0; Once you get rid of CronetEngine.INVALID_RTT_THROUGHPUT, these asserts wont be necessary. https://codereview.chromium.org/2417643007/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2417643007/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:786: public static final int INVALID_RTT_THROUGHPUT = Can you get rid of this and use RttThroughputValues.INVALID_RTT_THROUGHPUT_VALUE directly?
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
xunjieli: ptal. thanks. https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/160001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:366: checkHaveAdapter(); On 2016/10/19 23:32:27, xunjieli wrote: > On 2016/10/19 21:51:39, tbansal1 wrote: > > On 2016/10/18 21:52:00, xunjieli wrote: > > > Why do we need to checkHaveAdapter() here? Can we just return the result > > > directly? If the adapter isn't initialized or torn down, we will return > > invalid, > > > which seems reasonable. > > > > I suppose checking the adapter helps in catching errors. Just like it is > checked > > in other methods. > > In other places (except getEffectiveConnectionType()), checkHaveAdapter() makes > sure we are not calling into native code with a null object. Since we are not > calling into native code, there isn't a reason to incur the synchonization cost. > I'd suggest removing > synchronized(mLock) { > checkHaveAdapter(); > } Done. https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java (right): https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUrlRequestContextTest.java:284: assert CronetEngine.INVALID_RTT_THROUGHPUT < 0; On 2016/10/19 23:32:28, xunjieli wrote: > Once you get rid of CronetEngine.INVALID_RTT_THROUGHPUT, these asserts wont be > necessary. Done. https://codereview.chromium.org/2417643007/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/2417643007/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:786: public static final int INVALID_RTT_THROUGHPUT = On 2016/10/19 23:32:28, xunjieli wrote: > Can you get rid of this and use RttThroughputValues.INVALID_RTT_THROUGHPUT_VALUE > directly? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet_tester on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
Since no one said anything on the thread yet, I am assuming this is fine. LGTM. https://codereview.chromium.org/2417643007/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:96: @EffectiveConnectionType From the build output, this doesn't seem to work for some reason. I think we can leave it alone and revert it since it's not related to this CL.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/180001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:96: private int mEffectiveConnectionType = EffectiveConnectionType.TYPE_UNKNOWN; On 2016/10/19 23:32:27, xunjieli wrote: > Since you are here, could you add a @EffectiveConnectionType above this line? > > @EffectiveConnectionType > private int mEffectiveConnectionType = EffectiveConnectionType.TYPE_UNKNOWN; > > I believe it's legal coz we now have IntDef support in java_cpp_enum.py I think this gives a compilation error. See https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron... https://codereview.chromium.org/2417643007/diff/220001/components/cronet/andr... File components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java (right): https://codereview.chromium.org/2417643007/diff/220001/components/cronet/andr... components/cronet/android/java/src/org/chromium/net/impl/CronetUrlRequestContext.java:96: @EffectiveConnectionType On 2016/10/20 01:02:43, xunjieli wrote: > From the build output, this doesn't seem to work for some reason. I think we can > leave it alone and revert it since it's not related to this CL. Done in PS#7.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ryansturm@chromium.org, mgersh@chromium.org, xunjieli@chromium.org Link to the patchset: https://codereview.chromium.org/2417643007/#ps240001 (title: "Removed annotation")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
There were warnings when CQ was processing your CL: * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
There were warnings when CQ was processing your CL: * CQ is not running the android_cronet trybot, per your CQ_INCLUDE_TRYBOTS flag request. That bot was already specified
Message was sent while issue was closed.
Description was changed from ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=654498 ========== to ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=654498 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:240001)
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:240001) has been created in https://codereview.chromium.org/2433923005/ by tbansal@chromium.org. The reason for reverting is: Reverting this due to trybot failures: https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20ARM....
Message was sent while issue was closed.
A revert of this CL (patchset #7 id:240001) has been created in https://chromiumcodereview.appspot.com/2434273002/ by mathp@chromium.org. The reason for reverting is: Faliure to build. https://build.chromium.org/p/chromium.android/builders/Android%20Cronet%20ARM....
Message was sent while issue was closed.
Description was changed from ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=654498 ========== to ========== Expose RTT and throughput estimates from Cronet The estimates are computed by the network quality estimator (NQE) and are pushed to CronetUrlRequestContext. Embedders can query for the estimates by calling appropriate methods on CronetEngine. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=654498 Committed: https://crrev.com/dbd3adb57de9b1802723ddb1f27081ffd87eba73 Cr-Commit-Position: refs/heads/master@{#426517} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/dbd3adb57de9b1802723ddb1f27081ffd87eba73 Cr-Commit-Position: refs/heads/master@{#426517} |
