Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1889873005:
Record Lo-Fi NQE prediction accuracy at different intervals (Closed)
DescriptionRecord Lo-Fi NQE prediction accuracy at different intervals
When a main frame request is seen, the predicted network
quality is stored locally in a variable. After t seconds,
observed network quality is computed from all samples taken
in last t seconds. Accuracy is recorded based on comparison
between the predicted quality and the observed quality.
Currently, t is set to 15, 30 and 60 seconds.
The new histogram is of the format
"DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]"
The older histogram has been obsoleted.
BUG=603776
Committed: https://crrev.com/27cdb3ee8a113505f55cde29bb6a6a115d34a815
Cr-Commit-Position: refs/heads/master@{#388345}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : rebased #Patch Set 3 : megjablon comments #
Total comments: 20
Patch Set 4 : Addressed megjablon, asvitkine comments #Patch Set 5 : #
Total comments: 6
Patch Set 6 : addressed asvitkine comments #Messages
Total messages: 37 (22 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #2 (id:80001) has been deleted
Patchset #1 (id:60001) has been deleted
Description was changed from ========== Record LoFi NQE prediction accuracy at different intervals Record LoFi NQE prediction accuracy at different intervals BUG= ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals BUG=603776 ==========
Patchset #1 (id:100001) has been deleted
Description was changed from ========== Record Lo-Fi NQE prediction accuracy at different intervals BUG=603776 ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals Also, obsolete the older histogram. BUG=603776 ==========
Description was changed from ========== Record Lo-Fi NQE prediction accuracy at different intervals Also, obsolete the older histogram. BUG=603776 ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The older histogram is also obsoleted. BUG=603776 ==========
tbansal@chromium.org changed reviewers: + asvitkine@chromium.org, megjablon@chromium.org
megjablon: ptal at * asvitkine: ptal at histograms.xml.
Description was changed from ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The older histogram is also obsoleted. BUG=603776 ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram is also obsoleted. BUG=603776 ==========
Description was changed from ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram is also obsoleted. BUG=603776 ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram has been obsoleted. BUG=603776 ==========
Description was changed from ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram has been obsoleted. BUG=603776 ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram has been obsoleted. BUG=603776 ==========
https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:114: base::HistogramBase* GetEnumeratedHistogram( #include "base/metrics/histogram_base.h" https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:494: if (params::IsIncludedInLoFiEnabledFieldTrial() && Do you want to record this for the Control group too? It would give us double the metrics. https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (left): https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:88: // validate calls on the correct thread. |event_creator| is used for logging Why remove the |io_task_runner| description? Update it? https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:217: lofi_accuracy_recording_intervals() const; #include <vector> https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:113: std::vector<base::TimeDelta> lofi_accuracy_recording_intervals_; #include <vector> https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1180: // Tests if metrics for Lo-Fi accuracy are recorded properly. recorded properly at intervals?
megjablon: ptal. Thanks. https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:114: base::HistogramBase* GetEnumeratedHistogram( On 2016/04/16 00:09:34, megjablon wrote: > #include "base/metrics/histogram_base.h" Done. https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:494: if (params::IsIncludedInLoFiEnabledFieldTrial() && On 2016/04/16 00:09:34, megjablon wrote: > Do you want to record this for the Control group too? It would give us double > the metrics. Good point. Done. https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (left): https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:88: // validate calls on the correct thread. |event_creator| is used for logging On 2016/04/16 00:09:34, megjablon wrote: > Why remove the |io_task_runner| description? Update it? Not sure what to add. May be: |io_task_runner| is the IO task runner. But, this seems pretty obvious. I also do not want to write how |io_task_runner| is used by this class. I think header class should not describe its internal workings in comments. https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:217: lofi_accuracy_recording_intervals() const; On 2016/04/16 00:09:34, megjablon wrote: > #include <vector> Done. https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:113: std::vector<base::TimeDelta> lofi_accuracy_recording_intervals_; On 2016/04/16 00:09:35, megjablon wrote: > #include <vector> Done. https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc (right): https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:1180: // Tests if metrics for Lo-Fi accuracy are recorded properly. On 2016/04/16 00:09:35, megjablon wrote: > recorded properly at intervals? Done.
https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (left): https://chromiumcodereview.appspot.com/1889873005/diff/120001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:88: // validate calls on the correct thread. |event_creator| is used for logging On 2016/04/16 01:04:59, tbansal1 wrote: > On 2016/04/16 00:09:34, megjablon wrote: > > Why remove the |io_task_runner| description? Update it? > > Not sure what to add. May be: |io_task_runner| is the IO task runner. But, this > seems pretty obvious. > > I also do not want to write how |io_task_runner| is used by this class. I think > header class should not describe its internal workings in comments. |io_task_runner| must remain alive for the lifetime of the |DataReductionProxyConfig| instance? https://chromiumcodereview.appspot.com/1889873005/diff/160001/components/data... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://chromiumcodereview.appspot.com/1889873005/diff/160001/components/data... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:120: return base::Histogram::FactoryGet( #include "base/metrics/histogram.h"
https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:103: default: Nit: Omit the default cases and handle all the values in the enum. This way, if someone adds a new value they'll get a warning and will have to update the code. (In practice, change the default: to "case CONNECTION_LAST:" to handle all the cases.) Move the NOTREACHED() to the end of the function below the return ""; https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:844: const char prefix[] = "DataReductionProxy.LoFi.Accuracy."; Nit: For cont strings defined inside function, add "static". This avoids a bunch of extra generate binary code by the compiler (there was a thread about this on Chromium dev a couple years back). https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:846: prefix + base::IntToString(measuring_duration.InSeconds()) + ".", Can you add a DCHECK somewhere that measuring_duration.InSeconds() is one of the three supported values? (15/30/60) https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:300: // Records Lo-Fi accuracy metric. Add a comment about measuring_duration. Per histograms.xml, only three values are expected for it, right? 15/30/60 Actually, in that case, why pass it as a TimeDelta - maybe just an int? https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:95: const std::vector<base::TimeDelta>& lofi_accuracy_recording_intervals); Nit: trivial setters/getters using hacker_style convention should be defined inline. So either define inline or use CamelCase. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:98: const override; Nit: I think virtual functions shouldn't be using hacker_style syntax. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:100: void set_tick_clock(base::TickClock* tick_clock); Nit: Same comment as set_lofi_accuracy_recording_intervals(). https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:115: }; Nit: DISALLOW_COPY_AND_ASSIGN(). https://codereview.chromium.org/1889873005/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1889873005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88271: - <affected-histogram name="DataReductionProxy.AutoLoFiAccuracy"/> Mark the suffix as <obsolete>. See for e.g. CreateSession under EmePromise. Also, you can just not change it too, since it will inherit the obsoleteness of the base histogram.
Patchset #4 (id:180001) has been deleted
Patchset #4 (id:200001) has been deleted
megjablon, asvitkine: PTAL. Thanks! https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (left): https://codereview.chromium.org/1889873005/diff/120001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:88: // validate calls on the correct thread. |event_creator| is used for logging On 2016/04/18 18:58:49, megjablon wrote: > On 2016/04/16 01:04:59, tbansal1 wrote: > > On 2016/04/16 00:09:34, megjablon wrote: > > > Why remove the |io_task_runner| description? Update it? > > > > Not sure what to add. May be: |io_task_runner| is the IO task runner. But, > this > > seems pretty obvious. > > > > I also do not want to write how |io_task_runner| is used by this class. I > think > > header class should not describe its internal workings in comments. > > |io_task_runner| must remain alive for the lifetime of the > |DataReductionProxyConfig| instance? I do not think that's needed. The task runner is refcounted, so as long as DRP is holding a ref on it, the task runner won't be destroyed. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc (right): https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:103: default: On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Nit: Omit the default cases and handle all the values in the enum. This way, if > someone adds a new value they'll get a warning and will have to update the code. > > (In practice, change the default: to "case CONNECTION_LAST:" to handle all the > cases.) > > Move the NOTREACHED() to the end of the function below the return ""; Done. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:120: return base::Histogram::FactoryGet( On 2016/04/18 18:58:49, megjablon wrote: > #include "base/metrics/histogram.h" Done. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:844: const char prefix[] = "DataReductionProxy.LoFi.Accuracy."; On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Nit: For cont strings defined inside function, add "static". This avoids a bunch > of extra generate binary code by the compiler (there was a thread about this on > Chromium dev a couple years back). Done. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:846: prefix + base::IntToString(measuring_duration.InSeconds()) + ".", On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Can you add a DCHECK somewhere that measuring_duration.InSeconds() is one of the > three supported values? (15/30/60) Done. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:300: // Records Lo-Fi accuracy metric. On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Add a comment about measuring_duration. Per histograms.xml, only three values > are expected for it, right? 15/30/60 Done. > > Actually, in that case, why pass it as a TimeDelta - maybe just an int? Because this function compares delta between two time ticks with |measuring_duration|. If I pass seconds, I would need to upgrade that back to TimeDelta. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:95: const std::vector<base::TimeDelta>& lofi_accuracy_recording_intervals); On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Nit: trivial setters/getters using hacker_style convention should be defined > inline. So either define inline or use CamelCase. Done. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:98: const override; On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Nit: I think virtual functions shouldn't be using hacker_style syntax. Right, virtual functions can't be inline. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:100: void set_tick_clock(base::TickClock* tick_clock); On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Nit: Same comment as set_lofi_accuracy_recording_intervals(). Done. https://codereview.chromium.org/1889873005/diff/160001/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:115: }; On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Nit: DISALLOW_COPY_AND_ASSIGN(). Done. https://codereview.chromium.org/1889873005/diff/160001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1889873005/diff/160001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:88271: - <affected-histogram name="DataReductionProxy.AutoLoFiAccuracy"/> On 2016/04/18 20:35:09, Alexei Svitkine wrote: > Mark the suffix as <obsolete>. See for e.g. CreateSession under EmePromise. > > Also, you can just not change it too, since it will inherit the obsoleteness of > the base histogram. Done. Thanks, I was not sure if I could deprecate a specific suffix.
Patchset #5 (id:240001) has been deleted
lgtm % remaining comments https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:213: virtual const std::vector<base::TimeDelta>& LofiAccuracyRecordingIntervals() Nit: Get* https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:216: virtual base::TickClock* GetTickClock() const; Nit: If you always use this via GetTickClock()->NowTicks(), why not make this function just GetNowTicks()? Then, you don't even need the scoped_ptr<base::TickClock> tick_clock_; in the base class - you just need it in the test subclass. https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:101: void SetTickClock(base::TickClock* tick_clock); Nit: Add a comment about ownership somewhere?
megjablon: ptal . thanks! https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:213: virtual const std::vector<base::TimeDelta>& LofiAccuracyRecordingIntervals() On 2016/04/19 13:54:42, Alexei Svitkine wrote: > Nit: Get* Done. https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:216: virtual base::TickClock* GetTickClock() const; On 2016/04/19 13:54:41, Alexei Svitkine wrote: > Nit: If you always use this via GetTickClock()->NowTicks(), why not make this > function just GetNowTicks()? Then, you don't even need the > scoped_ptr<base::TickClock> tick_clock_; in the base class - you just need it in > the test subclass. God point. Done. https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:101: void SetTickClock(base::TickClock* tick_clock); On 2016/04/19 13:54:42, Alexei Svitkine wrote: > Nit: Add a comment about ownership somewhere? Done. I thought with pointers the default is to assume that callee does not get the ownership.
megjablon: ptal . thanks! https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h (right): https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:213: virtual const std::vector<base::TimeDelta>& LofiAccuracyRecordingIntervals() On 2016/04/19 13:54:42, Alexei Svitkine wrote: > Nit: Get* Done. https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:216: virtual base::TickClock* GetTickClock() const; On 2016/04/19 13:54:41, Alexei Svitkine wrote: > Nit: If you always use this via GetTickClock()->NowTicks(), why not make this > function just GetNowTicks()? Then, you don't even need the > scoped_ptr<base::TickClock> tick_clock_; in the base class - you just need it in > the test subclass. God point. Done. https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... File components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h (right): https://codereview.chromium.org/1889873005/diff/190011/components/data_reduct... components/data_reduction_proxy/core/browser/data_reduction_proxy_config_test_utils.h:101: void SetTickClock(base::TickClock* tick_clock); On 2016/04/19 13:54:42, Alexei Svitkine wrote: > Nit: Add a comment about ownership somewhere? Done. I thought with pointers the default is to assume that callee does not get the ownership.
lgtm
The CQ bit was checked by tbansal@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1889873005/#ps270001 (title: "addressed asvitkine comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889873005/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889873005/270001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by tbansal@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1889873005/270001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1889873005/270001
Message was sent while issue was closed.
Description was changed from ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram has been obsoleted. BUG=603776 ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram has been obsoleted. BUG=603776 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:270001)
Message was sent while issue was closed.
Description was changed from ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram has been obsoleted. BUG=603776 ========== to ========== Record Lo-Fi NQE prediction accuracy at different intervals When a main frame request is seen, the predicted network quality is stored locally in a variable. After t seconds, observed network quality is computed from all samples taken in last t seconds. Accuracy is recorded based on comparison between the predicted quality and the observed quality. Currently, t is set to 15, 30 and 60 seconds. The new histogram is of the format "DataReductionProxy.LoFi.Accuracy.[0-9]*.[ConnectionType]" The older histogram has been obsoleted. BUG=603776 Committed: https://crrev.com/27cdb3ee8a113505f55cde29bb6a6a115d34a815 Cr-Commit-Position: refs/heads/master@{#388345} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/27cdb3ee8a113505f55cde29bb6a6a115d34a815 Cr-Commit-Position: refs/heads/master@{#388345} |
