Extract the SafeSearch client to a separate directory

Extract the SafeSearch client to a separate directory

1. Extract SupervisedUserAsyncURLChecker and its unittest to a separate
directory to make it reusable. Rename it to SafeSearchURLChecker
as it's now available not only to supervised users, and as its
asynchronicity does not need to be stressed (as there is no synchronous
counterpart).

2. Decople it from the suprevised_users/ code; namely, introduce a
SafeSearchURLChecker::Classification enum that has a direct mapping to
SupervisedUserURLFilter::FilteringBehavior instead of using
FilteringBehavior directly.

3. Update the callsites in supervised_users/, headers, etc. No functional
changes to the code have been made in this CL.

BUG=653479
Committed: https://crrev.com/d8fd3b32ad7ee05cbfe2ada60e1d7086ac5e6255
Cr-Commit-Position: refs/heads/master@{#424149}

[msramek, 2016-10-06 11:24:31]

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

[msramek, 2016-10-06 11:24:32]

Hi Marc,

This is the separation of the SafeSearch client that we chatted about before.
Please have a look!

Thanks,
Martin

[msramek, 2016-10-06 11:30:00]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 11:30:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 12:24:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 12:24:07]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2016-10-06 13:16:42]

lgtm

Nice! Just a few nits.

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/OWNERS (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/OWNERS:1: treib@chromium.org
Please add bauerb@, and feel free to add yourself too if you want :)

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/safe_search_url_checker.cc (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.cc:114: Classification
classification,
nit: I think this fits on the previous line now. (Maybe just run "git cl
format"?)

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.cc:212: // TODO(msramek):
Should we rename the histogram?
Hm, probably. If not, then we should at least update the description in
histograms.xml.

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/safe_search_url_checker.h (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.h:28: enum Classification {
optional: Make this an enum class?

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

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:59: "a
FilteringBehavior interpretation.";
We'll get a compile error anyway if the switch doesn't handle all cases :)

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_url_filter.h (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.h:32: class
SafeSearchURLChecker;
Not needed if you include the header anyway.

[msramek, 2016-10-06 14:59:27]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 14:59:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2016-10-06 15:02:09]

Patchset #2 (id:20001) has been deleted

[msramek, 2016-10-06 15:02:30]

The CQ bit was checked by msramek@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 15:02:59]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[msramek, 2016-10-06 15:08:40]

msramek@chromium.org changed reviewers:
+ bauerb@chromium.org, jochen@chromium.org

[msramek, 2016-10-06 15:08:41]

Thanks Marc!

+Bernhard PTAL to confirm your inclusion in OWNERS

+Jochen PTAL for a new directory in c/b/ (the backstory is that we're starting a
project about curating browsing history, and classifying sites according to
SafeSearch is a part of that; Chrome already has a SafeSearch client, but we
want to decouple it from supervised users codebase first)

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/OWNERS (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/OWNERS:1: treib@chromium.org
On 2016/10/06 13:16:42, Marc Treib wrote:
> Please add bauerb@, and feel free to add yourself too if you want :)

Done.

I'll add myself once I know the code better. For now, I want to make sure that
my changes won't break any existing supervised_users/ functionality, so I will
want reviews from you anyway :)

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/safe_search_url_checker.cc (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.cc:114: Classification
classification,
On 2016/10/06 13:16:42, Marc Treib wrote:
> nit: I think this fits on the previous line now. (Maybe just run "git cl
> format"?)

Done. I intentionally didn't run format at first to minimize the number of
changes in this code review. It's fine in the second patchset though :)

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.cc:212: // TODO(msramek):
Should we rename the histogram?
On 2016/10/06 13:16:42, Marc Treib wrote:
> Hm, probably. If not, then we should at least update the description in
> histograms.xml.

Actually, now that I think about it, the "average safe search latency" and
"average supervised users page load delay" metrics might not be the same - it's
about how the clients use it, how much ends up going to the cache and so on.

I propose to move the metric to SupervisedUserResourceThrottle where the delay
actually happens (still leaving it as a TODO, let me know if you'd like me to do
it in this CL).

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/safe_search_url_checker.h (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.h:28: enum Classification {
On 2016/10/06 13:16:42, Marc Treib wrote:
> optional: Make this an enum class?

Done.

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

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:59: "a
FilteringBehavior interpretation.";
On 2016/10/06 13:16:42, Marc Treib wrote:
> We'll get a compile error anyway if the switch doesn't handle all cases :)

I know :) I just meant it as a comment and felt like putting it next to the
NOTREACHED() is a good place.

Let me just remove it, as the message is obvious. But I would keep the
NOTREACHED() to document that the "return" statement below is purely syntactical
and that it doesn't actually mean that BLOCK is the default.

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/supervised_u...
File chrome/browser/supervised_user/supervised_user_url_filter.h (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.h:32: class
SafeSearchURLChecker;
On 2016/10/06 13:16:42, Marc Treib wrote:
> Not needed if you include the header anyway.

Done.

[Marc Treib, 2016-10-06 15:17:15]

Thanks!

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/safe_search_url_checker.cc (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.cc:212: // TODO(msramek):
Should we rename the histogram?
On 2016/10/06 15:08:41, msramek wrote:
> On 2016/10/06 13:16:42, Marc Treib wrote:
> > Hm, probably. If not, then we should at least update the description in
> > histograms.xml.
> 
> Actually, now that I think about it, the "average safe search latency" and
> "average supervised users page load delay" metrics might not be the same -
it's
> about how the clients use it, how much ends up going to the cache and so on.

It's not a total page load delay, it's specifically the delay caused by the
SafeSites check, and I think it's only recorded for cache misses?

Anyway, this class should probably get some not-supervised-user-specific metrics
on latency, error rate etc., but that's for another CL.

> I propose to move the metric to SupervisedUserResourceThrottle where the delay
> actually happens (still leaving it as a TODO, let me know if you'd like me to
do
> it in this CL).

That SG. Another CL is fine; just make sure to move it out of here before
SafeSearchURLChecker is actually used anywhere else.

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

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/supervised_u...
chrome/browser/supervised_user/supervised_user_url_filter.cc:59: "a
FilteringBehavior interpretation.";
On 2016/10/06 15:08:41, msramek wrote:
> On 2016/10/06 13:16:42, Marc Treib wrote:
> > We'll get a compile error anyway if the switch doesn't handle all cases :)
> 
> I know :) I just meant it as a comment and felt like putting it next to the
> NOTREACHED() is a good place.
> 
> Let me just remove it, as the message is obvious. But I would keep the
> NOTREACHED() to document that the "return" statement below is purely
syntactical
> and that it doesn't actually mean that BLOCK is the default.

Yes, please do keep the NOTREACHED!

[Bernhard Bauer, 2016-10-06 15:21:12]

LGTM, but should we maybe call the directory safe_search_api?

[commit-bot: I haz the power, 2016-10-06 16:06:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 16:06:51]

Dry run: This issue passed the CQ dry run.

[msramek, 2016-10-07 08:54:52]

On 2016/10/06 15:21:12, Bernhard Bauer wrote:
> LGTM, but should we maybe call the directory safe_search_api?

To distinguish it from the "force SafeSearch on Google Search" option in //net ?
I guess that makes sense.

Done. (Only the directory name, build files, includes and include guards
changed; the diff unfortunately doesn't show that well).

[msramek, 2016-10-07 08:59:51]

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
File chrome/browser/safe_search/safe_search_url_checker.cc (right):

https://codereview.chromium.org/2399823002/diff/1/chrome/browser/safe_search/...
chrome/browser/safe_search/safe_search_url_checker.cc:212: // TODO(msramek):
Should we rename the histogram?
On 2016/10/06 15:17:15, Marc Treib wrote:
> On 2016/10/06 15:08:41, msramek wrote:
> > On 2016/10/06 13:16:42, Marc Treib wrote:
> > > Hm, probably. If not, then we should at least update the description in
> > > histograms.xml.
> > 
> > Actually, now that I think about it, the "average safe search latency" and
> > "average supervised users page load delay" metrics might not be the same -
> it's
> > about how the clients use it, how much ends up going to the cache and so on.
> 
> It's not a total page load delay, it's specifically the delay caused by the
> SafeSites check, and I think it's only recorded for cache misses?
> 
> Anyway, this class should probably get some not-supervised-user-specific
metrics
> on latency, error rate etc., but that's for another CL.
> 
> > I propose to move the metric to SupervisedUserResourceThrottle where the
delay
> > actually happens (still leaving it as a TODO, let me know if you'd like me
to
> do
> > it in this CL).
> 
> That SG. Another CL is fine; just make sure to move it out of here before
> SafeSearchURLChecker is actually used anywhere else.

Acknowledged. I made a note in the linked bug.

[jochen (gone - plz use gerrit), 2016-10-10 12:47:03]

lgtm

[msramek, 2016-10-10 12:50:16]

Thanks!

[msramek, 2016-10-10 12:50:22]

The CQ bit was checked by msramek@chromium.org

[msramek, 2016-10-10 12:50:22]

The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org,
bauerb@chromium.org
Link to the patchset: https://codereview.chromium.org/2399823002/#ps60001
(title: "Renamed to safe_search_api")

[commit-bot: I haz the power, 2016-10-10 12:50:33]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-10 14:36:39]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-10-10 14:39:07]

Description was changed from

==========
Extract the SafeSearch client to a separate directory

1. Extract SupervisedUserAsyncURLChecker and its unittest to a separate
directory to make it reusable. Rename it to SafeSearchURLChecker
as it's now available not only to supervised users, and as its
asynchronicity does not need to be stressed (as there is no synchronous
counterpart).

2. Decople it from the suprevised_users/ code; namely, introduce a
SafeSearchURLChecker::Classification enum that has a direct mapping to
SupervisedUserURLFilter::FilteringBehavior instead of using
FilteringBehavior directly.

3. Update the callsites in supervised_users/, headers, etc. No functional
changes to the code have been made in this CL.

BUG=653479
==========

to

==========
Extract the SafeSearch client to a separate directory

1. Extract SupervisedUserAsyncURLChecker and its unittest to a separate
directory to make it reusable. Rename it to SafeSearchURLChecker
as it's now available not only to supervised users, and as its
asynchronicity does not need to be stressed (as there is no synchronous
counterpart).

2. Decople it from the suprevised_users/ code; namely, introduce a
SafeSearchURLChecker::Classification enum that has a direct mapping to
SupervisedUserURLFilter::FilteringBehavior instead of using
FilteringBehavior directly.

3. Update the callsites in supervised_users/, headers, etc. No functional
changes to the code have been made in this CL.

BUG=653479

Committed: https://crrev.com/d8fd3b32ad7ee05cbfe2ada60e1d7086ac5e6255
Cr-Commit-Position: refs/heads/master@{#424149}
==========

[commit-bot: I haz the power, 2016-10-10 14:39:09]

Patchset 3 (id:??) landed as
https://crrev.com/d8fd3b32ad7ee05cbfe2ada60e1d7086ac5e6255
Cr-Commit-Position: refs/heads/master@{#424149}