Extract the SafeSearch client to a separate directory

chrome/browser/safe_search/safe_search_url_checker.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // 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>
 
 #include "base/callback.h"
 #include "base/json/json_reader.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/stl_util.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
@@ skipping 64 matching lines @@
   if (!classifications_list->GetDictionary(0, &classification_dict)) {
     DLOG(WARNING) << "ParseResponse failed to parse classification dict";
     return false;
   }
   classification_dict->GetBoolean("pornography", is_porn);
   return true;
 }
 
 }  // namespace
 
-struct SupervisedUserAsyncURLChecker::Check {
+struct SafeSearchURLChecker::Check {
   Check(const GURL& url,
         std::unique_ptr<net::URLFetcher> fetcher,
         const CheckCallback& callback);
   ~Check();
 
   GURL url;
   std::unique_ptr<net::URLFetcher> fetcher;
   std::vector<CheckCallback> callbacks;
   base::TimeTicks start_time;
 };
 
-SupervisedUserAsyncURLChecker::Check::Check(
+SafeSearchURLChecker::Check::Check(
     const GURL& url,
     std::unique_ptr<net::URLFetcher> fetcher,
     const CheckCallback& callback)
     : url(url),
       fetcher(std::move(fetcher)),
       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),
       cache_(cache_size),
       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
   // own Safety Mode anyway.
   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;
   }
 
   auto cache_it = cache_.Get(url);
   if (cache_it != cache_.end()) {
     const CheckResult& result = cache_it->second;
     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";
     cache_.Erase(cache_it);
   }
 
   // See if we already have a check in progress for this URL.
   for (Check* check : checks_in_progress_) {
     if (check->url == url) {
       DVLOG(1) << "Adding to pending check for " << url.spec();
       check->callbacks.push_back(callback);
       return false;
     }
   }
 
   DVLOG(1) << "Checking URL " << url;
   std::string api_key = google_apis::GetAPIKey();
   std::unique_ptr<URLFetcher> fetcher(
       CreateFetcher(this, context_, api_key, url));
   fetcher->Start();
   checks_in_progress_.push_back(new Check(url, std::move(fetcher), callback));
   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()) {
     if (source == (*it)->fetcher.get())
       break;
     ++it;
   }
   DCHECK(it != checks_in_progress_.end());
   Check* check = *it;
 
   const URLRequestStatus& status = source->GetStatus();
   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;
   }
 
   std::string response_body;
   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);
 }