Extract the SafeSearch client to a separate directory

chrome/browser/safe_search/safe_search_url_checker.cc

Index: chrome/browser/safe_search/safe_search_url_checker.cc
diff --git a/chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc b/chrome/browser/safe_search/safe_search_url_checker.cc
similarity index 81%
rename from chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
rename to chrome/browser/safe_search/safe_search_url_checker.cc
index d29df9b576b629667486ba4a5f9198a66e0a9939..ec37ff37b9b3744039834274ff25c227cc105792 100644
--- a/chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.cc
+++ b/chrome/browser/safe_search/safe_search_url_checker.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "chrome/browser/supervised_user/experimental/supervised_user_async_url_checker.h"
+#include "chrome/browser/safe_search/safe_search_url_checker.h"
 
 #include <string>
 #include <utility>
@@ -87,7 +87,7 @@ bool ParseResponse(const std::string& response, bool* is_porn) {
 
 }  // namespace
 
-struct SupervisedUserAsyncURLChecker::Check {
+struct SafeSearchURLChecker::Check {
   Check(const GURL& url,
         std::unique_ptr<net::URLFetcher> fetcher,
         const CheckCallback& callback);
@@ -99,7 +99,7 @@ struct SupervisedUserAsyncURLChecker::Check {
   base::TimeTicks start_time;
 };
 
-SupervisedUserAsyncURLChecker::Check::Check(
+SafeSearchURLChecker::Check::Check(
     const GURL& url,
     std::unique_ptr<net::URLFetcher> fetcher,
     const CheckCallback& callback)
@@ -108,20 +108,20 @@ SupervisedUserAsyncURLChecker::Check::Check(
       callbacks(1, callback),
       start_time(base::TimeTicks::Now()) {}
 
-SupervisedUserAsyncURLChecker::Check::~Check() {}
+SafeSearchURLChecker::Check::~Check() {}
 
-SupervisedUserAsyncURLChecker::CheckResult::CheckResult(
-    SupervisedUserURLFilter::FilteringBehavior behavior,
+SafeSearchURLChecker::CheckResult::CheckResult(
+    Classification classification,

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

nit: I think this fits on the previous line now. (Maybe just run "git cl
format"?)

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

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 :)

     bool uncertain)
-    : behavior(behavior),
+    : classification(classification),
       uncertain(uncertain),
       timestamp(base::TimeTicks::Now()) {}
 
-SupervisedUserAsyncURLChecker::SupervisedUserAsyncURLChecker(
+SafeSearchURLChecker::SafeSearchURLChecker(
     URLRequestContextGetter* context)
-    : SupervisedUserAsyncURLChecker(context, kDefaultCacheSize) {}
+    : SafeSearchURLChecker(context, kDefaultCacheSize) {}
 
-SupervisedUserAsyncURLChecker::SupervisedUserAsyncURLChecker(
+SafeSearchURLChecker::SafeSearchURLChecker(
     URLRequestContextGetter* context,
     size_t cache_size)
     : context_(context),
@@ -129,16 +129,16 @@ SupervisedUserAsyncURLChecker::SupervisedUserAsyncURLChecker(
       cache_timeout_(
           base::TimeDelta::FromSeconds(kDefaultCacheTimeoutSeconds)) {}
 
-SupervisedUserAsyncURLChecker::~SupervisedUserAsyncURLChecker() {}
+SafeSearchURLChecker::~SafeSearchURLChecker() {}
 
-bool SupervisedUserAsyncURLChecker::CheckURL(const GURL& url,
-                                             const CheckCallback& callback) {
+bool SafeSearchURLChecker::CheckURL(const GURL& url,
+                                    const CheckCallback& callback) {
   // TODO(treib): Hack: For now, allow all Google URLs to save QPS. If we ever
   // remove this, we should find a way to allow at least the NTP.
   if (google_util::IsGoogleDomainUrl(url,
                                      google_util::ALLOW_SUBDOMAIN,
                                      google_util::ALLOW_NON_STANDARD_PORTS)) {
-    callback.Run(url, SupervisedUserURLFilter::ALLOW, false);
+    callback.Run(url, SAFE, false);
     return true;
   }
   // TODO(treib): Hack: For now, allow all YouTube URLs since YouTube has its
@@ -146,7 +146,7 @@ bool SupervisedUserAsyncURLChecker::CheckURL(const GURL& url,
   if (google_util::IsYoutubeDomainUrl(url,
                                       google_util::ALLOW_SUBDOMAIN,
                                       google_util::ALLOW_NON_STANDARD_PORTS)) {
-    callback.Run(url, SupervisedUserURLFilter::ALLOW, false);
+    callback.Run(url, SAFE, false);
     return true;
   }
 
@@ -156,10 +156,9 @@ bool SupervisedUserAsyncURLChecker::CheckURL(const GURL& url,
     base::TimeDelta age = base::TimeTicks::Now() - result.timestamp;
     if (age < cache_timeout_) {
       DVLOG(1) << "Cache hit! " << url.spec() << " is "
-               << (result.behavior == SupervisedUserURLFilter::BLOCK ? "NOT"
-                                                                     : "")
+               << (result.classification == UNSAFE ? "NOT" : "")
                << " safe; certain: " << !result.uncertain;
-      callback.Run(url, result.behavior, result.uncertain);
+      callback.Run(url, result.classification, result.uncertain);
       return true;
     }
     DVLOG(1) << "Outdated cache entry for " << url.spec() << ", purging";
@@ -184,7 +183,7 @@ bool SupervisedUserAsyncURLChecker::CheckURL(const GURL& url,
   return false;
 }
 
-void SupervisedUserAsyncURLChecker::OnURLFetchComplete(
+void SafeSearchURLChecker::OnURLFetchComplete(
     const net::URLFetcher* source) {
   ScopedVector<Check>::iterator it = checks_in_progress_.begin();
   while (it != checks_in_progress_.end()) {
@@ -199,7 +198,7 @@ void SupervisedUserAsyncURLChecker::OnURLFetchComplete(
   if (!status.is_success()) {
     DLOG(WARNING) << "URL request failed! Letting through...";
     for (size_t i = 0; i < check->callbacks.size(); i++)
-      check->callbacks[i].Run(check->url, SupervisedUserURLFilter::ALLOW, true);
+      check->callbacks[i].Run(check->url, SAFE, true);
     checks_in_progress_.erase(it);
     return;
   }
@@ -208,15 +207,15 @@ void SupervisedUserAsyncURLChecker::OnURLFetchComplete(
   source->GetResponseAsString(&response_body);
   bool is_porn = false;
   bool uncertain = !ParseResponse(response_body, &is_porn);
-  SupervisedUserURLFilter::FilteringBehavior behavior =
-      is_porn ? SupervisedUserURLFilter::BLOCK : SupervisedUserURLFilter::ALLOW;
+  Classification classification = is_porn ? UNSAFE : SAFE;
 
+  // TODO(msramek): Should we rename the histogram?

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

Hm, probably. If not, then we should at least update the description in
histograms.xml.

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

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).

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

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.
That SG. Another CL is fine; just make sure to move it out of here before
SafeSearchURLChecker is actually used anywhere else.

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

Acknowledged. I made a note in the linked bug.

   UMA_HISTOGRAM_TIMES("ManagedUsers.SafeSitesDelay",
                       base::TimeTicks::Now() - check->start_time);
 
-  cache_.Put(check->url, CheckResult(behavior, uncertain));
+  cache_.Put(check->url, CheckResult(classification, uncertain));
 
   for (size_t i = 0; i < check->callbacks.size(); i++)
-    check->callbacks[i].Run(check->url, behavior, uncertain);
+    check->callbacks[i].Run(check->url, classification, uncertain);
   checks_in_progress_.erase(it);
 }