Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2906723002:
Track how often we would attempt to resolve non-'compliant' domain names. (Closed)
DescriptionTrack how often we would attempt to resolve non-'compliant' domain names.
We have 2 hostname goodness predicates: checking that
`net::IsValidHostLabelCharacter` returns true for all characters in all labels,
and `net::IsCanonicalizedHostCompliant`. The latter is more strict than the
former, in that it imposes an additional constraint on the final label: it can
only begin with an alphanumeric character.
Add a counter to see how often hostnames would pass the former check but fail
the latter, stricter check. This will help us decide which predicate to finalize
on.
BUG=695474
Review-Url: https://codereview.chromium.org/2906723002
Cr-Commit-Position: refs/heads/master@{#476122}
Committed: https://chromium.googlesource.com/chromium/src/+/64af80e692404a380476b4b7edb27392eae75014
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fix description of histogram. #
Total comments: 2
Patch Set 3 : Respond to comments. #
Total comments: 2
Patch Set 4 : Respond to comments. #Messages
Total messages: 21 (9 generated)
palmer@chromium.org changed reviewers: + mpearson@chromium.org, pkasting@chromium.org, rsleevi@chromium.org
rsleevi: For net mpearson: For histograms.xml pkasting: FYI
The CQ bit was checked by palmer@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...
LGTM https://codereview.chromium.org/2906723002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2906723002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:40036: + returns true. Used to see if we can deprecate and remove support for Is this second sentence really accurate? It sounds like it might be a copy-and-paste and should have been something like "Used to see if IsCanonicalizedHostCompliant() runs afoul of real websites."
https://codereview.chromium.org/2906723002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2906723002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:40036: + returns true. Used to see if we can deprecate and remove support for On 2017/05/25 at 20:03:30, Peter Kasting wrote: > Is this second sentence really accurate? It sounds like it might be a copy-and-paste and should have been something like "Used to see if IsCanonicalizedHostCompliant() runs afoul of real websites." Done!
lgtm
https://codereview.chromium.org/2906723002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2906723002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40035: + True if Net.ValidDNSName is true and if |net::IsCanonicalizedHostCompliant| This description doesn't sound right. It sounds like the value emitted is Net.ValidDNSName() && net::IsCanonicalizedHostCompliant() Looking at the code, that's not correct. This value is only emitted if you've decided it's a valid DNS name, and the value emitted is net::IsCanonicalizedHostCompliant(). Please rewrite, thanks. Also, the histogram probably needs a better name. The current one implies the && interpretation.
And, side-comment: I recently reviewed an authentication histogram that records invalid machine names in that context. You may want to look at the data coming back from that histogram too. https://chromium-review.googlesource.com/514042 cheers, mark
https://codereview.chromium.org/2906723002/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2906723002/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40035: + True if Net.ValidDNSName is true and if |net::IsCanonicalizedHostCompliant| On 2017/05/26 at 17:27:02, Mark P wrote: > This description doesn't sound right. It sounds like the value emitted is > Net.ValidDNSName() && net::IsCanonicalizedHostCompliant() > > Looking at the code, that's not correct. This value is only emitted if you've decided it's a valid DNS name, and the value emitted is net::IsCanonicalizedHostCompliant(). > > Please rewrite, thanks. > > Also, the histogram probably needs a better name. The current one implies the && interpretation. Done.
> I recently reviewed an authentication histogram that records invalid machine names in that context. > You may want to look at the data coming back from that histogram too. > https://chromium-review.googlesource.com/514042 Great, thank you.
The CQ bit was checked by palmer@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...
lgtm modulo comment --mark https://codereview.chromium.org/2906723002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2906723002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35394: + True if Net.ValidDNSName is true and if |net::IsCanonicalizedHostCompliant| nit: I think this makes more sense if you omit "if Net.ValidDNSName is true and " from this sentence. I think it's still misleading until you read to the end.
https://codereview.chromium.org/2906723002/diff/40001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2906723002/diff/40001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:35394: + True if Net.ValidDNSName is true and if |net::IsCanonicalizedHostCompliant| > nit: I think this makes more sense if you omit "if Net.ValidDNSName is true and " from this sentence. I think it's still misleading until you read to the end. Done.
The CQ bit was checked by palmer@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, rsleevi@chromium.org, mpearson@chromium.org Link to the patchset: https://codereview.chromium.org/2906723002/#ps60001 (title: "Respond to comments.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1496273863913750,
"parent_rev": "728cff0c8b3f6e205d5ab5d26e63df7e67e55d29", "commit_rev":
"64af80e692404a380476b4b7edb27392eae75014"}
Message was sent while issue was closed.
Description was changed from ========== Track how often we would attempt to resolve non-'compliant' domain names. We have 2 hostname goodness predicates: checking that `net::IsValidHostLabelCharacter` returns true for all characters in all labels, and `net::IsCanonicalizedHostCompliant`. The latter is more strict than the former, in that it imposes an additional constraint on the final label: it can only begin with an alphanumeric character. Add a counter to see how often hostnames would pass the former check but fail the latter, stricter check. This will help us decide which predicate to finalize on. BUG=695474 ========== to ========== Track how often we would attempt to resolve non-'compliant' domain names. We have 2 hostname goodness predicates: checking that `net::IsValidHostLabelCharacter` returns true for all characters in all labels, and `net::IsCanonicalizedHostCompliant`. The latter is more strict than the former, in that it imposes an additional constraint on the final label: it can only begin with an alphanumeric character. Add a counter to see how often hostnames would pass the former check but fail the latter, stricter check. This will help us decide which predicate to finalize on. BUG=695474 Review-Url: https://codereview.chromium.org/2906723002 Cr-Commit-Position: refs/heads/master@{#476122} Committed: https://chromium.googlesource.com/chromium/src/+/64af80e692404a380476b4b7edb2... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/64af80e692404a380476b4b7edb2... |
