Identify localhost/ipaddress in RapporSamples

Identify localhost/ipaddress in RapporSamples

BUG=459419
Committed: https://crrev.com/9f9f038ba648fa01256edba94cb23bd23dbf9136
Cr-Commit-Position: refs/heads/master@{#317389}

[Steven Holte, 2015-02-18 02:40:16]

holte@chromium.org changed reviewers:
+ nparker@chromium.org

[Alexei Svitkine (slow), 2015-02-18 21:22:19]

asvitkine@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Alexei Svitkine (slow), 2015-02-18 21:22:20]

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
File chrome/browser/metrics/rappor/sampling.cc (right):

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling.cc:16: if (net::IsLocalhost(gurl.host()))
{
Nit: No {}'s. Same below.

[Nathan Parker, 2015-02-18 21:56:46]

Awesome, thanks.

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_metrics_helper.cc
(right):

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_metrics_helper.cc:73: //
|domain| will be empty for hosts w/o TLDs (localhost, ip addrs)
Update this comment.  I think it's true still except localhost and ipaddrs.

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
File chrome/browser/metrics/rappor/sampling.cc (right):

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling.cc:17: return "localhost";
I think this should go inside the if IsHTTPorHTTPS, so then we know "localhost"
doesn't mean some odd scheme

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
File chrome/browser/metrics/rappor/sampling_unittest.cc (right):

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling_unittest.cc:29:
GURL("http://[2001:db8::1]/")));
While you're here, add two more:

http://www/ --> ""
http://www.corp/ --> "corp" (I think)

[Steven Holte, 2015-02-19 23:42:19]

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/interstit...
File chrome/browser/interstitials/security_interstitial_metrics_helper.cc
(right):

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/interstit...
chrome/browser/interstitials/security_interstitial_metrics_helper.cc:73: //
|domain| will be empty for hosts w/o TLDs (localhost, ip addrs)
On 2015/02/18 21:56:46, nparker wrote:
> Update this comment.  I think it's true still except localhost and ipaddrs.

Done.

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
File chrome/browser/metrics/rappor/sampling.cc (right):

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling.cc:16: if (net::IsLocalhost(gurl.host()))
{
On 2015/02/18 21:22:20, Alexei Svitkine wrote:
> Nit: No {}'s. Same below.

Done.

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling.cc:17: return "localhost";
On 2015/02/18 21:56:46, nparker wrote:
> I think this should go inside the if IsHTTPorHTTPS, so then we know
"localhost"
> doesn't mean some odd scheme

Done.

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
File chrome/browser/metrics/rappor/sampling_unittest.cc (right):

https://codereview.chromium.org/933853003/diff/40001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling_unittest.cc:29:
GURL("http://[2001:db8::1]/")));
On 2015/02/18 21:56:46, nparker wrote:
> While you're here, add two more:
> 
> http://www/ --> ""
> http://www.corp/ --> "corp" (I think)

Done, but http://www.corp/ --> "www.corp"

[Alexei Svitkine (slow), 2015-02-20 18:32:37]

lgtm

[Nathan Parker, 2015-02-20 18:47:04]

lgtm

https://codereview.chromium.org/933853003/diff/60001/chrome/browser/metrics/r...
File chrome/browser/metrics/rappor/sampling.cc (right):

https://codereview.chromium.org/933853003/diff/60001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling.cc:20: return "ip_address";
Will the Rappor backend match against these strings? (not sure what is the
correct terminology for that matching).

[felt, 2015-02-20 19:07:17]

felt@chromium.org changed reviewers:
+ felt@chromium.org

[felt, 2015-02-20 19:07:17]

rubber stamp lgtm for
chrome/browser/interstitials/security_interstitial_metrics_helper.cc

[Steven Holte, 2015-02-20 19:47:11]

https://codereview.chromium.org/933853003/diff/60001/chrome/browser/metrics/r...
File chrome/browser/metrics/rappor/sampling.cc (right):

https://codereview.chromium.org/933853003/diff/60001/chrome/browser/metrics/r...
chrome/browser/metrics/rappor/sampling.cc:20: return "ip_address";
On 2015/02/20 18:47:04, nparker wrote:
> Will the Rappor backend match against these strings? (not sure what is the
> correct terminology for that matching).

Yes, I'll just add these to the list of strings we test for.

[Steven Holte, 2015-02-20 19:48:01]

The CQ bit was checked by holte@chromium.org

[commit-bot: I haz the power, 2015-02-20 19:48:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/933853003/60001

[commit-bot: I haz the power, 2015-02-20 20:31:19]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-20 20:32:19]

Patchset 4 (id:??) landed as
https://crrev.com/9f9f038ba648fa01256edba94cb23bd23dbf9136
Cr-Commit-Position: refs/heads/master@{#317389}