Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2491703003:
NQE: Notify observer as soon as it is added (Closed)
DescriptionNQE: Notify observer as soon as it is added
When an EffectiveConnectionTypeObserver or an
RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator
(NQE), it is notified of the current effective connection type or
current RTT/throughput estimates.
Doing this makes the API simpler (the caller does not have to
call Get*() functions when registering as an observer). It also
reduces the need of exposing both Get*() and Add*() APIs to different
layers of Chromium.
BUG=638304
Committed: https://crrev.com/286d35bb3f13b27ada977c535d35fbe3977b9426
Cr-Commit-Position: refs/heads/master@{#432637}
Patch Set 1 : ps #
Total comments: 2
Patch Set 2 : ryansturm comments #
Total comments: 2
Patch Set 3 : Moar tests #
Total comments: 6
Patch Set 4 : Addressed bengr comments #Patch Set 5 : Addressed bengr comments #
Messages
Total messages: 46 (30 generated)
Description was changed from ========== Notify observer as soon as it is added BUG= ========== to ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. BUG=638304 ==========
Patchset #1 (id:1) has been deleted
Description was changed from ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. BUG=638304 ========== to ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=638304 ==========
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. thanks.
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.
This is a little weird. Would it make more sense to return the ECT instead of immediately post a task to call into it? What is the motivation behind this?
Description was changed from ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=638304 ========== to ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. Doing this makes the API simpler (the caller does not have to call Get*() functions when registering as an observer). It also reduces the need of exposing both Get*() and Add*() APIs to different layers of Chromium. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=638304 ==========
On 2016/11/10 21:25:25, Ryan Sturm wrote: > This is a little weird. Would it make more sense to return the ECT instead of > immediately post a task to call into it? What is the motivation behind this? I added a bit of details in the CL description. Currently, it is possible to register as an RTT/throughput change observer. However, NQE does not expose an API to get the current value of RTT and throughput. So, it is not possible to get the initial values. Arguably, I can add 3 APIs to expose them, but I think this is simpler. It also reduces the chances of error on the part of observer, in case they forget to call the Get* function before registering.
Add parity to the UI thread API that does the same thing. https://codereview.chromium.org/2491703003/diff/20001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2491703003/diff/20001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:208: void UINetworkQualityEstimatorService::AddEffectiveConnectionTypeObserver( Add the same behavior for this.
Patchset #2 (id:40001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
ptal. thanks. https://codereview.chromium.org/2491703003/diff/20001/chrome/browser/net/nqe/... File chrome/browser/net/nqe/ui_network_quality_estimator_service.cc (right): https://codereview.chromium.org/2491703003/diff/20001/chrome/browser/net/nqe/... chrome/browser/net/nqe/ui_network_quality_estimator_service.cc:208: void UINetworkQualityEstimatorService::AddEffectiveConnectionTypeObserver( On 2016/11/10 22:21:34, Ryan Sturm wrote: > Add the same behavior for this. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm % testing corner cases. https://codereview.chromium.org/2491703003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2491703003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1521: if (!effective_connection_type_observer_list_.HasObserver(observer)) Add testing for Add then Remove observer (not crashing) and ECT = UNKNOWN not sending an update.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Patchset #3 (id:80001) has been deleted
Patchset #3 (id:100001) has been deleted
https://codereview.chromium.org/2491703003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2491703003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1521: if (!effective_connection_type_observer_list_.HasObserver(observer)) On 2016/11/10 23:07:22, Ryan Sturm wrote: > Add testing for Add then Remove observer (not crashing) and ECT = UNKNOWN not > sending an update. Done.
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. thanks.
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.
https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1153: base::ThreadTaskRunnerHandle::Get()->PostTask( The real question is why do you post tasks for all of these instead of calling directly? https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:218: // effective connection type in the next message pump, if it is still I think it's overkill to say "if it is still registered as an observer." I would think that that's the contract that AddECTObserver and RemoveECTObserver provide. Likewise at lines 230 ,499, and 505.
https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1153: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/11/11 17:26:44, bengr wrote: > The real question is why do you post tasks for all of these instead of calling > directly? I like avoiding weird re-entrancy issues. Seems like if adding the observer was part of initialization, the observer might not be fully set up to handle the callback immediately.
bengr: ptal. Thanks. https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1153: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/11/11 17:30:18, Ryan Sturm wrote: > On 2016/11/11 17:26:44, bengr wrote: > > The real question is why do you post tasks for all of these instead of calling > > directly? > > I like avoiding weird re-entrancy issues. Seems like if adding the observer was > part of initialization, the observer might not be fully set up to handle the > callback immediately. Right, the caller may not have been set up fully. https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:218: // effective connection type in the next message pump, if it is still On 2016/11/11 17:26:44, bengr wrote: > I think it's overkill to say "if it is still registered as an observer." I would > think that that's the contract that AddECTObserver and RemoveECTObserver > provide. Likewise at lines 230 ,499, and 505. Done.
bengr: ping.
lgtm, with request for comment. https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2491703003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:1153: base::ThreadTaskRunnerHandle::Get()->PostTask( On 2016/11/11 18:20:45, tbansal1 wrote: > On 2016/11/11 17:30:18, Ryan Sturm wrote: > > On 2016/11/11 17:26:44, bengr wrote: > > > The real question is why do you post tasks for all of these instead of > calling > > > directly? > > > > I like avoiding weird re-entrancy issues. Seems like if adding the observer > was > > part of initialization, the observer might not be fully set up to handle the > > callback immediately. > > Right, the caller may not have been set up fully. Please add a comment somewhere that says you post so as not to require the caller to be fully set up before registering an observer.
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...)
Description was changed from ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. Doing this makes the API simpler (the caller does not have to call Get*() functions when registering as an observer). It also reduces the need of exposing both Get*() and Add*() APIs to different layers of Chromium. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester BUG=638304 ========== to ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. Doing this makes the API simpler (the caller does not have to call Get*() functions when registering as an observer). It also reduces the need of exposing both Get*() and Add*() APIs to different layers of Chromium. BUG=638304 ==========
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, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2491703003/#ps160001 (title: "Addressed bengr comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. Doing this makes the API simpler (the caller does not have to call Get*() functions when registering as an observer). It also reduces the need of exposing both Get*() and Add*() APIs to different layers of Chromium. BUG=638304 ========== to ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. Doing this makes the API simpler (the caller does not have to call Get*() functions when registering as an observer). It also reduces the need of exposing both Get*() and Add*() APIs to different layers of Chromium. BUG=638304 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. Doing this makes the API simpler (the caller does not have to call Get*() functions when registering as an observer). It also reduces the need of exposing both Get*() and Add*() APIs to different layers of Chromium. BUG=638304 ========== to ========== NQE: Notify observer as soon as it is added When an EffectiveConnectionTypeObserver or an RTTAndThroughputEstimatesObserver is added to NetworkQualityEstimator (NQE), it is notified of the current effective connection type or current RTT/throughput estimates. Doing this makes the API simpler (the caller does not have to call Get*() functions when registering as an observer). It also reduces the need of exposing both Get*() and Add*() APIs to different layers of Chromium. BUG=638304 Committed: https://crrev.com/286d35bb3f13b27ada977c535d35fbe3977b9426 Cr-Commit-Position: refs/heads/master@{#432637} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/286d35bb3f13b27ada977c535d35fbe3977b9426 Cr-Commit-Position: refs/heads/master@{#432637} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
