Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2421063002:
NQE: Separate out params to a different file (Closed)
DescriptionNQE: Separate out params to a different file
There is no functionality change. This CL only moves the
params and the related functions to a separate file.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet
BUG=638308
Committed: https://crrev.com/7a718ef951d41df73fdf7e00725c919498bf6394
Cr-Commit-Position: refs/heads/master@{#428822}
Patch Set 1 : ps #
Total comments: 14
Patch Set 2 : Addressed ryansturm comments #Patch Set 3 : Rebased #Patch Set 4 : Initialize variation params value to avoid compilation error #
Total comments: 22
Patch Set 5 : Rebased #Patch Set 6 : More rebasing #Patch Set 7 : Addressed bengr comments #
Messages
Total messages: 61 (42 generated)
Description was changed from ========== Move params out BUG= ========== to ========== Move params out 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 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...
Description was changed from ========== Move params out CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG= ========== to ========== NQE: Move params out CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG= ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) 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...
Patchset #1 (id:20001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== NQE: Move params out CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG= ========== to ========== NQE: Move params out There is no functional change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ==========
tbansal@chromium.org changed reviewers: + ryansturm@chromium.org
ptal. thanks.
Description was changed from ========== NQE: Move params out There is no functional change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ========== to ========== NQE: Separate out params to a different file There is no functional change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ==========
Description was changed from ========== NQE: Separate out params to a different file There is no functional change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ========== to ========== NQE: Separate out params to a different file There is no functionality change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ==========
https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_params.cc (right): https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:65: void GetValueForVariationParam( Return an int64_t instead of void and an out param? https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:88: double variations_value = default_value; why define this as default_value? https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:136: return exp(log(0.5) / half_life_seconds); nit: I think this would be more readable as pow(.5, 1.0 / half_life_seconds). To me, that reads as "If I raise the returned value to the |half_life_seconds|, I will get .5". WDYT? I would doubt there are performance implications either way. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:167: nqe::internal::NetworkQuality default_observations[]) { I don't think you should pass this as an array, but rather a pointer and access it the same way. I don't particularly like the use of array here because array doesn't carry a size with it. Maybe you should switch to a vector? https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:209: nqe::internal::NetworkQuality connection_thresholds[]) { same as comment above.
Patchset #2 (id:60001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by tbansal@chromium.org to run a CQ dry run
ryansturm: ptal. thanks. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_params.cc (right): https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:65: void GetValueForVariationParam( On 2016/10/18 23:29:04, Ryan Sturm wrote: > Return an int64_t instead of void and an out param? Done. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:88: double variations_value = default_value; On 2016/10/18 23:29:04, Ryan Sturm wrote: > why define this as default_value? Done. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:136: return exp(log(0.5) / half_life_seconds); On 2016/10/18 23:29:04, Ryan Sturm wrote: > nit: I think this would be more readable as pow(.5, 1.0 / half_life_seconds). To > me, that reads as "If I raise the returned value to the |half_life_seconds|, I > will get .5". WDYT? I would doubt there are performance implications either way. > Done. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:167: nqe::internal::NetworkQuality default_observations[]) { On 2016/10/18 23:29:04, Ryan Sturm wrote: > I don't think you should pass this as an array, but rather a pointer and access > it the same way. I don't particularly like the use of array here because array > doesn't carry a size with it. Maybe you should switch to a vector? The size of the array has to always match the number of ECTs. So, the size is known (although out of band) at the compile time, and the size never changes. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:209: nqe::internal::NetworkQuality connection_thresholds[]) { On 2016/10/18 23:29:04, Ryan Sturm wrote: > same as comment above. See the reply above.
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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
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.
lgtm % 1 comment (comment is a nit at this point, so feel free to leave it as is if there is a reason) https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_params.cc (right): https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:88: double variations_value = default_value; On 2016/10/20 01:02:15, tbansal1 wrote: > On 2016/10/18 23:29:04, Ryan Sturm wrote: > > why define this as default_value? > > Done. I guess I was asking why variations_value = default_value; is done here. If StringToDouble succeeds variations_value will be set, and if it fails, it will return default_value, so there is no need to set it initially. Same thing on line 70 now. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:167: nqe::internal::NetworkQuality default_observations[]) { On 2016/10/20 01:02:15, tbansal1 wrote: > On 2016/10/18 23:29:04, Ryan Sturm wrote: > > I don't think you should pass this as an array, but rather a pointer and > access > > it the same way. I don't particularly like the use of array here because array > > doesn't carry a size with it. Maybe you should switch to a vector? > > The size of the array has to always match the number of ECTs. So, the size is > known (although out of band) at the compile time, and the size never changes. Acknowledged.
https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_params.cc (right): https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:88: double variations_value = default_value; On 2016/10/20 17:29:59, Ryan Sturm wrote: > On 2016/10/20 01:02:15, tbansal1 wrote: > > On 2016/10/18 23:29:04, Ryan Sturm wrote: > > > why define this as default_value? > > > > Done. > > I guess I was asking why variations_value = default_value; is done here. If > StringToDouble succeeds variations_value will be set, and if it fails, it will > return default_value, so there is no need to set it initially. > > Same thing on line 70 now. Windows compiler complains otherwise. Look at the trybots log from PS 3. I thought I had added a comment here explaining this, but I forgot.
tbansal@chromium.org changed reviewers: + bengr@chromium.org
bengr: ptal. thanks.
Level 4 warnings are dumb. I'm curious if what I said below would work, but it might not. https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... File net/nqe/network_quality_estimator_params.cc (right): https://codereview.chromium.org/2421063002/diff/40001/net/nqe/network_quality... net/nqe/network_quality_estimator_params.cc:88: double variations_value = default_value; On 2016/10/20 17:32:14, tbansal1 wrote: > On 2016/10/20 17:29:59, Ryan Sturm wrote: > > On 2016/10/20 01:02:15, tbansal1 wrote: > > > On 2016/10/18 23:29:04, Ryan Sturm wrote: > > > > why define this as default_value? > > > > > > Done. > > > > I guess I was asking why variations_value = default_value; is done here. If > > StringToDouble succeeds variations_value will be set, and if it fails, it will > > return default_value, so there is no need to set it initially. > > > > Same thing on line 70 now. > > Windows compiler complains otherwise. Look at the trybots log from PS 3. I > thought I had added a comment here explaining this, but I forgot. If you're feeling curious, I bet that switching the return statements and removing the ! Will fix it on windows compilers, but that is just a hunch.
https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:60: prefix + statistic_name + You could just move "NQE." to this line. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_params.cc (right): https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:52: const char kThresholdKbpsSuffix[] = ".ThresholdMedianKbps"; Why do these variable names not match the string literals? I.e., why is this one not kThresholdMedianKbpsSuffix? https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:68: int64_t default_value) { #include <stdint.h> https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:168: for (size_t i = 0; i <= NetworkChangeNotifier::CONNECTION_LAST; ++i) { i should include CONNECTION_LAST? https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:212: [EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_LAST]; Do you need the prefix? https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_params.h (right): https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:27: // Computes and returns the weight multiplier per second. You should define what a "weight multiplier per second" is. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:29: // related to NetworkQualityEstimator field trial. to -> to the https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:37: // Sets the default observation for different connection types in Define what a "default observation" is. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:49: // Returns the probability at which the correlation UMA should be recorded. This could be worded better. Maybe something like: "Returns the fraction of events that should record correlation UMA." https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:53: // Returns true if effective connection type value has been forced via variation if -> if the type value -> type forced -> determined? https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:58: // Returns the effective connection type set via variation parameters. type set via -> type that was configured by
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...
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
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
The CQ bit was unchecked by commit-bot@chromium.org
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...)
bengr: ptal. thanks. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator.cc (right): https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator.cc:60: prefix + statistic_name + On 2016/10/27 16:12:29, bengr wrote: > You could just move "NQE." to this line. Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_params.cc (right): https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:52: const char kThresholdKbpsSuffix[] = ".ThresholdMedianKbps"; On 2016/10/27 16:12:29, bengr wrote: > Why do these variable names not match the string literals? I.e., why is this one > not kThresholdMedianKbpsSuffix? Removed them since they were used only at one place. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:68: int64_t default_value) { On 2016/10/27 16:12:29, bengr wrote: > #include <stdint.h> Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:168: for (size_t i = 0; i <= NetworkChangeNotifier::CONNECTION_LAST; ++i) { On 2016/10/27 16:12:29, bengr wrote: > i should include CONNECTION_LAST? Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.cc:212: [EffectiveConnectionType::EFFECTIVE_CONNECTION_TYPE_LAST]; On 2016/10/27 16:12:29, bengr wrote: > Do you need the prefix? Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... File net/nqe/network_quality_estimator_params.h (right): https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:27: // Computes and returns the weight multiplier per second. On 2016/10/27 16:12:30, bengr wrote: > You should define what a "weight multiplier per second" is. Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:29: // related to NetworkQualityEstimator field trial. On 2016/10/27 16:12:30, bengr wrote: > to -> to the Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:37: // Sets the default observation for different connection types in On 2016/10/27 16:12:30, bengr wrote: > Define what a "default observation" is. Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:49: // Returns the probability at which the correlation UMA should be recorded. On 2016/10/27 16:12:30, bengr wrote: > This could be worded better. Maybe something like: > "Returns the fraction of events that should record correlation UMA." Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:53: // Returns true if effective connection type value has been forced via variation On 2016/10/27 16:12:29, bengr wrote: > if -> if the > type value -> type > forced -> determined? Done. https://codereview.chromium.org/2421063002/diff/140001/net/nqe/network_qualit... net/nqe/network_quality_estimator_params.h:58: // Returns the effective connection type set via variation parameters. On 2016/10/27 16:12:30, bengr wrote: > type set via -> type that was configured by Done.
lgtm
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 Link to the patchset: https://codereview.chromium.org/2421063002/#ps190001 (title: "Addressed bengr comments")
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
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by tbansal@chromium.org
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 ========== NQE: Separate out params to a different file There is no functionality change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ========== to ========== NQE: Separate out params to a different file There is no functionality change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== NQE: Separate out params to a different file There is no functionality change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 ========== to ========== NQE: Separate out params to a different file There is no functionality change. This CL only moves the params and the related functions to a separate file. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester;master.tryserver.chromium.android:android_cronet BUG=638308 Committed: https://crrev.com/7a718ef951d41df73fdf7e00725c919498bf6394 Cr-Commit-Position: refs/heads/master@{#428822} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7a718ef951d41df73fdf7e00725c919498bf6394 Cr-Commit-Position: refs/heads/master@{#428822} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
