Supervised users: Add histogram to record filtering results

Supervised users: Add histogram to record filtering results

This is in addition to the SafeSites histogram.

Also, cleanup some unused histograms in the URL filter.

Committed: https://crrev.com/7856c1e2e4e46b68a24339c0cce14a0ec0baf804
Cr-Commit-Position: refs/heads/master@{#324238}

[Marc Treib, 2015-03-31 09:01:17]

treib@chromium.org changed reviewers:
+ pam@chromium.org

[Marc Treib, 2015-03-31 09:01:18]

https://codereview.chromium.org/1043113002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_url_filter.cc (left):

https://codereview.chromium.org/1043113002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:420:
UMA_HISTOGRAM_CUSTOM_COUNTS("ManagedMode.ManualHostsEntries",
As far as I can tell, these were never defined in the data files. If no one has
missed them so far, then we probably don't need them (or we're just getting the
data from the dashboard).

[Marc Treib, 2015-04-01 09:29:23]

treib@chromium.org changed reviewers:
+ bauerb@chromium.org

[Marc Treib, 2015-04-01 09:29:23]

+bauerb

[Bernhard Bauer, 2015-04-07 11:03:30]

LGTM w/ a nit:

https://codereview.chromium.org/1043113002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_resource_throttle.cc
(right):

https://codereview.chromium.org/1043113002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:59: if
(reason == SupervisedUserURLFilter::BLACKLIST)
Make this a switch statement?

[Marc Treib, 2015-04-07 11:40:56]

https://codereview.chromium.org/1043113002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_resource_throttle.cc
(right):

https://codereview.chromium.org/1043113002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:59: if
(reason == SupervisedUserURLFilter::BLACKLIST)
On 2015/04/07 11:03:29, Bernhard Bauer wrote:
> Make this a switch statement?

Done.

[Marc Treib, 2015-04-07 11:41:45]

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

[Marc Treib, 2015-04-07 11:41:45]

+asvitkine for histograms.xml OWNERS approval. PTAL!

[Bernhard Bauer, 2015-04-07 11:43:48]

https://codereview.chromium.org/1043113002/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_resource_throttle.cc
(right):

https://codereview.chromium.org/1043113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:69: // Fall
through.
We don't really have a case where we fall through now, right?

https://codereview.chromium.org/1043113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:70: default:
You could also replace this with the last value (INVALID), so the compiler will
check for you that you're covering all the cases.

[Marc Treib, 2015-04-07 11:52:59]

https://codereview.chromium.org/1043113002/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_resource_throttle.cc
(right):

https://codereview.chromium.org/1043113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:69: // Fall
through.
On 2015/04/07 11:43:47, Bernhard Bauer wrote:
> We don't really have a case where we fall through now, right?

Right. If we ever add a new value, the compiler should complain here :)

https://codereview.chromium.org/1043113002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:70: default:
On 2015/04/07 11:43:48, Bernhard Bauer wrote:
> You could also replace this with the last value (INVALID), so the compiler
will
> check for you that you're covering all the cases.

Done.

[Bernhard Bauer, 2015-04-07 12:14:42]

Great, thanks!

[Alexei Svitkine (slow), 2015-04-07 15:05:49]

https://codereview.chromium.org/1043113002/diff/40001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_resource_throttle.cc
(right):

https://codereview.chromium.org/1043113002/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:102:
UMA_HISTOGRAM_ENUMERATION("ManagedUsers.FilteringResult",
Please change both of these to UMA_HISTOGRAM_SPARSE_SLOWLY, which is more
appropriate to use for enumerations that have so many distinct values (avoids
pre-allocating 700 buckets).

[Marc Treib, 2015-04-07 15:20:09]

https://codereview.chromium.org/1043113002/diff/40001/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_resource_throttle.cc
(right):

https://codereview.chromium.org/1043113002/diff/40001/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_resource_throttle.cc:102:
UMA_HISTOGRAM_ENUMERATION("ManagedUsers.FilteringResult",
On 2015/04/07 15:05:49, Alexei Svitkine wrote:
> Please change both of these to UMA_HISTOGRAM_SPARSE_SLOWLY, which is more
> appropriate to use for enumerations that have so many distinct values (avoids
> pre-allocating 700 buckets).

Today I learned.
Thanks, done!

[Alexei Svitkine (slow), 2015-04-07 18:43:05]

lgtm

[Marc Treib, 2015-04-08 08:03:09]

The CQ bit was checked by treib@chromium.org

[Marc Treib, 2015-04-08 08:03:09]

The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/1043113002/#ps60001
(title: "sparse histogram")

[commit-bot: I haz the power, 2015-04-08 08:03:32]

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

[commit-bot: I haz the power, 2015-04-08 17:03:06]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-08 17:03:57]

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