Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1222)

Issue 1797313003: Add a histogram (Net.PacResultForStrippedUrl) that estimates how often PAC scripts depend on the UR… (Closed)

Created:
4 years, 9 months ago by eroman
Modified:
4 years, 9 months ago
Reviewers:
cbentzel, Mark P
CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Add a histogram (Net.PacResultForStrippedUrl) that estimates how often PAC scripts depend on the URL path. BUG=593759 Committed: https://crrev.com/183f4bcddd821b4af3f6234ad2e8c371694a9ffb Cr-Commit-Position: refs/heads/master@{#382644}

Patch Set 1 #

Patch Set 2 : rebase #

Total comments: 6

Patch Set 3 : address cbentzel feedback #

Total comments: 8

Patch Set 4 : Improve comments and histogram description #

Total comments: 2

Patch Set 5 : Update some more comments per mpearson's feedback. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+468 lines, -10 lines) Patch
A + net/data/proxy_resolver_v8_tracing_unittest/alert_url.js View 1 1 chunk +4 lines, -1 line 0 comments Download
A net/data/proxy_resolver_v8_tracing_unittest/dns_depending_on_url.js View 1 chunk +17 lines, -0 lines 0 comments Download
A net/data/proxy_resolver_v8_tracing_unittest/error_depending_on_url.js View 1 chunk +13 lines, -0 lines 0 comments Download
A net/data/proxy_resolver_v8_tracing_unittest/return_url_as_proxy.js View 1 chunk +16 lines, -0 lines 0 comments Download
M net/proxy/proxy_resolver_v8_tracing.h View 1 2 3 4 1 chunk +49 lines, -0 lines 0 comments Download
M net/proxy/proxy_resolver_v8_tracing.cc View 1 2 9 chunks +142 lines, -0 lines 0 comments Download
M net/proxy/proxy_resolver_v8_tracing_unittest.cc View 15 chunks +190 lines, -9 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 2 chunks +37 lines, -0 lines 0 comments Download

Messages

Total messages: 17 (5 generated)
eroman
I can send this to a different reviewer, but figured since you were on the ...
4 years, 9 months ago (2016-03-16 08:32:34 UTC) #2
cbentzel
LGTM https://codereview.chromium.org/1797313003/diff/20001/net/proxy/proxy_resolver_v8_tracing.cc File net/proxy/proxy_resolver_v8_tracing.cc (right): https://codereview.chromium.org/1797313003/diff/20001/net/proxy/proxy_resolver_v8_tracing.cc#newcode640 net/proxy/proxy_resolver_v8_tracing.cc:640: // not check whether the script succeeds when ...
4 years, 9 months ago (2016-03-18 18:54:53 UTC) #3
eroman
https://codereview.chromium.org/1797313003/diff/20001/net/proxy/proxy_resolver_v8_tracing.cc File net/proxy/proxy_resolver_v8_tracing.cc (right): https://codereview.chromium.org/1797313003/diff/20001/net/proxy/proxy_resolver_v8_tracing.cc#newcode640 net/proxy/proxy_resolver_v8_tracing.cc:640: // not check whether the script succeeds when using ...
4 years, 9 months ago (2016-03-18 20:20:17 UTC) #4
eroman
+mpearson for histograms.xml approval.
4 years, 9 months ago (2016-03-18 20:23:55 UTC) #6
Mark P
https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc File net/proxy/proxy_resolver_v8_tracing.cc (right): https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc#newcode79 net/proxy/proxy_resolver_v8_tracing.cc:79: kHistogramPacResultForStrippedUrl, static_cast<int>(value), Why bother with the constant if it's ...
4 years, 9 months ago (2016-03-18 21:34:50 UTC) #7
eroman
https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc File net/proxy/proxy_resolver_v8_tracing.cc (right): https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc#newcode79 net/proxy/proxy_resolver_v8_tracing.cc:79: kHistogramPacResultForStrippedUrl, static_cast<int>(value), On 2016/03/18 21:34:50, Mark P wrote: > ...
4 years, 9 months ago (2016-03-18 23:15:16 UTC) #8
eroman
ping
4 years, 9 months ago (2016-03-22 15:23:00 UTC) #9
Mark P
histograms.xml lgtm with two minor comments below https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc File net/proxy/proxy_resolver_v8_tracing.cc (right): https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc#newcode79 net/proxy/proxy_resolver_v8_tracing.cc:79: kHistogramPacResultForStrippedUrl, static_cast<int>(value), ...
4 years, 9 months ago (2016-03-22 17:46:22 UTC) #10
eroman
https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc File net/proxy/proxy_resolver_v8_tracing.cc (right): https://codereview.chromium.org/1797313003/diff/40001/net/proxy/proxy_resolver_v8_tracing.cc#newcode79 net/proxy/proxy_resolver_v8_tracing.cc:79: kHistogramPacResultForStrippedUrl, static_cast<int>(value), On 2016/03/22 17:46:22, Mark P wrote: > ...
4 years, 9 months ago (2016-03-22 18:07:07 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1797313003/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1797313003/80001
4 years, 9 months ago (2016-03-22 18:07:49 UTC) #14
commit-bot: I haz the power
Committed patchset #5 (id:80001)
4 years, 9 months ago (2016-03-22 19:39:17 UTC) #15
commit-bot: I haz the power
4 years, 9 months ago (2016-03-22 19:41:08 UTC) #17
Message was sent while issue was closed.
Patchset 5 (id:??) landed as
https://crrev.com/183f4bcddd821b4af3f6234ad2e8c371694a9ffb
Cr-Commit-Position: refs/heads/master@{#382644}

Powered by Google App Engine
This is Rietveld 408576698