Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2406763003:
Add Network Quality observer to NQE (Closed)
DescriptionAdd Network Quality observer to NQE.
Network quality (HTTP RTT, transport RTT, downstream
throughput) is computed every time effective connection
type is computed. The observer is notified of a change in the
network quality only if the effective connection type also
changed since the last notification.
BUG=654498
Committed: https://crrev.com/dfcbdf56a8e6a7d21d71ffead1c27cfa64a0da3a
Cr-Commit-Position: refs/heads/master@{#425230}
Patch Set 1 : ps #
Total comments: 14
Patch Set 2 : Ryan's comments, Also send notifications more often #
Total comments: 2
Patch Set 3 : typo fix #
Total comments: 6
Patch Set 4 : Addressed bengr comments #Patch Set 5 : Fix presubmit errors (FOR_EACH_OBSERVER is deprecated) #
Messages
Total messages: 45 (33 generated)
Description was changed from ========== Add Network Quality observer to NQE. Add Network Quality observer to NQE. BUG= ========== to ========== Add Network Quality observer to NQE. Network quality (HTTP RTT, transport RTT, downstream throughput) is computed every time effective connection type is computed. The observer is notified of a change in the network quality only if the effective connection type also changed since the last notification. BUG=654498 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) 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:40001) 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...
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ryansturm: ptal. Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1225: if (past_type != effective_connection_type_) What about comparing old RTT and kbps? Should there just be a separate NotifyObserversOfNetworkQualityChanged method that gets called in that case? https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:98: // be notified on the IO thread. nit: I feel like this comment could be reformatted (i.e. register in lline 97 might fit on the previous line). I didn't count characters or anything, so I might be wrong. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:100: // If the computed network quality is different from the previously notified nit: Can you say something like "If any of the three components of network quality are different" ? I imagine in practice it is rare one changes and another stays the same, but it sounds like network quality is a metric from which rtt and kbps are derived as opposed to rtt and kbps being components of network quality. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:356: // |http_rtt_metric|, |transport_rtt_metric| and Are the "|*_metric|" variables relevant to this method? https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:659: // Observer list for changes in the effective connection type. nit: I don't think adding "the" here improves this comment. I prefer it the way it was, but I don't think it makes a large difference. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_test_util.h (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_test_util.h:87: // then the base implementation is called. Add comments about how |http_rtt|, |transport_rtt| and |downstream_throughput_kbps| are set. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:94: size_t notifications_received_; nit: Why size_t here and not int? There's not really a size since this isn't a list of notifications for instance.
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: This issue passed the CQ dry run.
Patchset #2 (id:80001) has been deleted
Patchset #2 (id:100001) has been deleted
ryansturm: ptal. Thanks. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.cc:1225: if (past_type != effective_connection_type_) On 2016/10/11 01:46:26, Ryan Sturm wrote: > What about comparing old RTT and kbps? Should there just be a separate > NotifyObserversOfNetworkQualityChanged method that gets called in that case? Done. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:98: // be notified on the IO thread. On 2016/10/11 01:46:26, Ryan Sturm wrote: > nit: I feel like this comment could be reformatted (i.e. register in lline 97 > might fit on the previous line). I didn't count characters or anything, so I > might be wrong. Done. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:100: // If the computed network quality is different from the previously notified On 2016/10/11 01:46:26, Ryan Sturm wrote: > nit: Can you say something like "If any of the three components of network > quality are different" ? I imagine in practice it is rare one changes and > another stays the same, but it sounds like network quality is a metric from > which rtt and kbps are derived as opposed to rtt and kbps being components of > network quality. Done. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:356: // |http_rtt_metric|, |transport_rtt_metric| and On 2016/10/11 01:46:26, Ryan Sturm wrote: > Are the "|*_metric|" variables relevant to this method? Done. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator.h:659: // Observer list for changes in the effective connection type. On 2016/10/11 01:46:26, Ryan Sturm wrote: > nit: I don't think adding "the" here improves this comment. I prefer it the way > it was, but I don't think it makes a large difference. Done. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_test_util.h (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_test_util.h:87: // then the base implementation is called. On 2016/10/11 01:46:26, Ryan Sturm wrote: > Add comments about how |http_rtt|, |transport_rtt| and > |downstream_throughput_kbps| are set. Done. https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2406763003/diff/60001/net/nqe/network_quality... net/nqe/network_quality_estimator_unittest.cc:94: size_t notifications_received_; On 2016/10/11 01:46:26, Ryan Sturm wrote: > nit: Why size_t here and not int? There's not really a size since this isn't a > list of notifications for instance. Done. Not sure it matters though. From https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md: "Use size_t for object and allocation sizes, object counts, ..."
lgtm % nit. https://codereview.chromium.org/2406763003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2406763003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:91: // estimated downstream throughput) is computed. NetworkQualityEstimator s/throughput)/throughput/
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. https://codereview.chromium.org/2406763003/diff/120001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2406763003/diff/120001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:91: // estimated downstream throughput) is computed. NetworkQualityEstimator On 2016/10/11 18:02:59, Ryan Sturm wrote: > s/throughput)/throughput/ Done.
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.
lgtm with nits https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:506: // Notify the observers of RTT or throughput estimates computation. Notify -> Notifies https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:1420: // Next request should not trigger recomputation of RTT or throughput Next -> The next https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:1429: // Change in connection type should send out notification to the observers. Change in connection type -> A Chrome in the connection type
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/2406763003/#ps160001 (title: "Addressed bengr comments")
https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.h (right): https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator.h:506: // Notify the observers of RTT or throughput estimates computation. On 2016/10/13 23:30:31, bengr wrote: > Notify -> Notifies Done. https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_unittest.cc (right): https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:1420: // Next request should not trigger recomputation of RTT or throughput On 2016/10/13 23:30:32, bengr wrote: > Next -> The next Done. https://codereview.chromium.org/2406763003/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_unittest.cc:1429: // Change in connection type should send out notification to the observers. On 2016/10/13 23:30:31, bengr wrote: > Change in connection type -> A Chrome in the connection type Done.
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
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
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: 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 bengr@chromium.org, ryansturm@chromium.org Link to the patchset: https://codereview.chromium.org/2406763003/#ps180001 (title: "Fix presubmit errors (FOR_EACH_OBSERVER is deprecated)")
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 ========== Add Network Quality observer to NQE. Network quality (HTTP RTT, transport RTT, downstream throughput) is computed every time effective connection type is computed. The observer is notified of a change in the network quality only if the effective connection type also changed since the last notification. BUG=654498 ========== to ========== Add Network Quality observer to NQE. Network quality (HTTP RTT, transport RTT, downstream throughput) is computed every time effective connection type is computed. The observer is notified of a change in the network quality only if the effective connection type also changed since the last notification. BUG=654498 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Add Network Quality observer to NQE. Network quality (HTTP RTT, transport RTT, downstream throughput) is computed every time effective connection type is computed. The observer is notified of a change in the network quality only if the effective connection type also changed since the last notification. BUG=654498 ========== to ========== Add Network Quality observer to NQE. Network quality (HTTP RTT, transport RTT, downstream throughput) is computed every time effective connection type is computed. The observer is notified of a change in the network quality only if the effective connection type also changed since the last notification. BUG=654498 Committed: https://crrev.com/dfcbdf56a8e6a7d21d71ffead1c27cfa64a0da3a Cr-Commit-Position: refs/heads/master@{#425230} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/dfcbdf56a8e6a7d21d71ffead1c27cfa64a0da3a Cr-Commit-Position: refs/heads/master@{#425230} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
