Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
566943004:
DataReductionProxy.BypassedBytes.* counts all triggering request bytes (Closed)
DescriptionDataReductionProxy.BypassedBytes.* counts all triggering request bytes
All DataReductionProxy.BypassedBytes.* UMA that don't have an explicit triggering
request UMA need to count the triggering request bytes.
BUG=413771
Committed: https://crrev.com/3d6e847d28e399f4ae8cd879c7df1a3b3baa1e52
Cr-Commit-Position: refs/heads/master@{#295848}
Patch Set 1 : #
Total comments: 2
Patch Set 2 : Adding comment #
Total comments: 10
Patch Set 3 : Addressing bengr comments #Patch Set 4 : Sync and rebase #Messages
Total messages: 17 (6 generated)
Patchset #1 (id:1) has been deleted
megjablon@chromium.org changed reviewers: + bengr@chromium.org
https://codereview.chromium.org/566943004/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/566943004/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:201: if (triggering_request_ && Please add a comment here that explains why we only enter this block on SHORT, MEDIUM, and LONG bypass types.
https://codereview.chromium.org/566943004/diff/20001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/566943004/diff/20001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:201: if (triggering_request_ && On 2014/09/16 01:55:05, bengr1 wrote: > Please add a comment here that explains why we only enter this block on SHORT, > MEDIUM, and LONG bypass types. Done.
https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:201: // We only record separate triggering request UMA for short, medium, and long Don't use "We" in comments. Change to: Triggering request UMA are recorded separately only for short, medium and long bypass events. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:207: // We only record when audio or video triggers a bypass. We don't care Remove the "We"s in this comment too. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:212: // media type is audio or video. Add a comment that states that we only have a bucket for short bypasses due to a/v. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:216: RecordBypassedBytes(last_bypass_type_, This UMA is complicated enough that we should have unit tests for all of it. Please file a bug with the label Cr-Internals-Network-DataProxy and assign yourself. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:237: AreDataReductionProxiesBypassed(request, NULL)) { Can the line break be before "request" instead of after "->"?
Patchset #3 (id:60001) has been deleted
https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... File components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc (right): https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:201: // We only record separate triggering request UMA for short, medium, and long On 2014/09/19 17:23:33, bengr1 wrote: > Don't use "We" in comments. Change to: > > Triggering request UMA are recorded separately only for short, medium and long > bypass events. Done. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:207: // We only record when audio or video triggers a bypass. We don't care On 2014/09/19 17:23:33, bengr1 wrote: > Remove the "We"s in this comment too. Done. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:212: // media type is audio or video. On 2014/09/19 17:23:33, bengr1 wrote: > Add a comment that states that we only have a bucket for short bypasses due to > a/v. Done. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:216: RecordBypassedBytes(last_bypass_type_, On 2014/09/19 17:23:33, bengr1 wrote: > This UMA is complicated enough that we should have unit tests for all of it. > Please file a bug with the label Cr-Internals-Network-DataProxy and assign > yourself. Done. https://codereview.chromium.org/566943004/diff/40001/components/data_reductio... components/data_reduction_proxy/browser/data_reduction_proxy_usage_stats.cc:237: AreDataReductionProxiesBypassed(request, NULL)) { On 2014/09/19 17:23:33, bengr1 wrote: > Can the line break be before "request" instead of after "->"? Done.
lgtm
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/566943004/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) win_chromium_rel_swarming on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by megjablon@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/566943004/100001
Message was sent while issue was closed.
Committed patchset #4 (id:100001) as 99c234ef896f1a0def635922eb5ee94274f3db2b
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/3d6e847d28e399f4ae8cd879c7df1a3b3baa1e52 Cr-Commit-Position: refs/heads/master@{#295848} |
