Offline blacklisting for SuggestionsService.

chrome/browser/search/suggestions/suggestions_service.cc

Index: chrome/browser/search/suggestions/suggestions_service.cc
diff --git a/chrome/browser/search/suggestions/suggestions_service.cc b/chrome/browser/search/suggestions/suggestions_service.cc
index d4e47452c8498f4857c595f9f96d5118654bbe6f..0fd34b18e0ee2793fee0a620c8488acb5c5731d2 100644
--- a/chrome/browser/search/suggestions/suggestions_service.cc
+++ b/chrome/browser/search/suggestions/suggestions_service.cc
@@ -4,6 +4,8 @@
 
 #include "chrome/browser/search/suggestions/suggestions_service.h"
 
+#include <string>
+
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
@@ -14,6 +16,7 @@
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/metrics/variations/variations_http_header_provider.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/search/suggestions/blacklist_store.h"
 #include "chrome/browser/search/suggestions/suggestions_store.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/variations/variations_associated_data.h"
@@ -21,6 +24,7 @@
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/base/net_errors.h"
+#include "net/base/url_util.h"
 #include "net/http/http_response_headers.h"
 #include "net/http/http_status_code.h"
 #include "net/http/http_util.h"
@@ -62,7 +66,7 @@ void DispatchRequestsAndClear(
     std::vector<SuggestionsService::ResponseCallback>* requestors) {
   std::vector<SuggestionsService::ResponseCallback>::iterator it;
   for (it = requestors->begin(); it != requestors->end(); ++it) {
-    it->Run(suggestions);
+    if (!it->is_null()) it->Run(suggestions);
   }
   std::vector<SuggestionsService::ResponseCallback>().swap(*requestors);
 }
@@ -72,6 +76,18 @@ void DispatchRequestsAndClear(
 // TODO(manzagop): make this a Variations parameter to enable tweaking.
 const unsigned int kRequestTimeoutMs = 200;
 
+// Initial delay used when scheduling a blacklist request.

[Mathieu, 2014/06/17 14:51:15]

nit: Initial -> Default?

Also remove Initial from variable name

[manzagop (departed), 2014/06/17 15:42:23]

Done.

+const unsigned int kBlacklistInitialDelaySec = 1;
+
+// Multiplier on the delay used when scheduling a blacklist request, in case the
+// last observed request was unsuccessful.
+const unsigned int kBlacklistBackoffMultiplier = 2;
+
+// Maximum valid delay for scheduling a request. Candidate delays larger than
+// this are rejected. This means the maximum backoff is at least 5400 / 2, ie

[Mathieu, 2014/06/17 14:51:15]

*i.e.

[manzagop (departed), 2014/06/17 15:42:23]

Done.

+// 45 minutes.
+const unsigned int kBacklistMaxDelaySec = 5400;

[Mathieu, 2014/06/17 14:51:14]

*Blacklist

[Mathieu, 2014/06/17 14:51:15]

How do you pick this value? I think it should be much shorter i.e. 5-10 minutes,
because sessions on mobile are never 45mn

[manzagop (departed), 2014/06/17 15:42:23]

Can this keep the phone awake and nuke the battery?

[manzagop (departed), 2014/06/17 15:42:23]

Done.

[Mathieu, 2014/06/17 19:54:36]

I don't think so, because this will not run if Chrome is not in the foreground
(we could check though). However if it's too high, it will in effect never run.
I said 5mn because network conditions could have changed in those 5mn.

+
 }  // namespace
 
 const char kSuggestionsFieldTrialName[] = "ChromeSuggestions";
@@ -83,10 +99,13 @@ const char kSuggestionsFieldTrialStateParam[] = "state";
 const char kSuggestionsFieldTrialStateEnabled[] = "enabled";
 
 SuggestionsService::SuggestionsService(

[Mathieu, 2014/06/17 14:51:14]

I think somewhere in the construction path (or something) we should schedule a
blacklist upload.

[manzagop (departed), 2014/06/17 15:42:23]

An upload will happen after the first attempt to get suggestions. Are you
worried it will end up with too few suggestions because of client side
blacklisting?

[Mathieu, 2014/06/17 19:54:36]

No, but we should make sure that the blacklist doesn't get too large. By the
way, you should add histograms for the size of the blacklist, when
SuggestionService is constructed.

UMA_HISTOGRAM_SPARSE_SLOWLY I think is a good macro for that.

-    Profile* profile, scoped_ptr<SuggestionsStore> suggestions_store)
+    Profile* profile, scoped_ptr<SuggestionsStore> suggestions_store,
+    scoped_ptr<BlacklistStore> blacklist_store)
     : suggestions_store_(suggestions_store.Pass()),
+      blacklist_store_(blacklist_store.Pass()),
       thumbnail_manager_(new ThumbnailManager(profile)),
       profile_(profile),
+      blacklist_delay_sec_(kBlacklistInitialDelaySec),
       weak_ptr_factory_(this) {
   // Obtain the URL to use to fetch suggestions data from the Variations param.
   suggestions_url_ =
@@ -132,10 +151,7 @@ void SuggestionsService::FetchSuggestionsDataNoTimeout(
   // Form new request.
   DCHECK(waiting_requestors_.empty());
   waiting_requestors_.push_back(callback);
-
-  pending_request_.reset(CreateSuggestionsRequest(suggestions_url_));
-  pending_request_->Start();
-  last_request_started_time_ = base::TimeTicks::Now();
+  IssueRequest(suggestions_url_);
 }
 
 void SuggestionsService::GetPageThumbnail(
@@ -145,38 +161,70 @@ void SuggestionsService::GetPageThumbnail(
 }
 
 void SuggestionsService::BlacklistURL(
-    const GURL& candidate_url, SuggestionsService::ResponseCallback callback) {
+    const GURL& candidate_url,
+    const SuggestionsService::ResponseCallback& callback) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   waiting_requestors_.push_back(callback);
 
-  if (pending_request_.get()) {
-    if (IsBlacklistRequest(pending_request_.get())) {
-      // Pending request is a blacklist request. Silently drop the new blacklist
-      // request. TODO - handle this case.
-      return;
-    } else {
-      // Pending request is not a blacklist request - cancel it and go on to
-      // issuing a blacklist request. Also ensure the timeout closure does not
-      // run; instead we'll wait for the updated suggestions before servicing
-      // requestors.
-      pending_request_.reset(NULL);
-      pending_timeout_closure_.reset(NULL);
-    }
-  }
+  // Blacklist locally, for immediate effect.
+  if (!blacklist_store_->BlacklistUrl(candidate_url))
+    DVLOG(1) << "Failed blacklisting attempt.";
+
+  // If there's an ongoing request, let it complete.
+  if (pending_request_.get()) return;
 
   // Send blacklisting request.
-  // TODO(manzagop): make this a PUT request instead of a GET request.
   GURL url(blacklist_url_prefix_ +
            net::EscapeQueryParamValue(candidate_url.spec(), true));
-  pending_request_.reset(CreateSuggestionsRequest(url));
-  pending_request_->Start();
-  last_request_started_time_ = base::TimeTicks::Now();
+  IssueRequest(url);
+}
+
+void SuggestionsService::UploadBlacklist() {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // If there's an ongoing request, let it complete.
+  if (pending_request_.get()) return;
+
+  GURL blacklist_url;
+  if (!blacklist_store_->GetSomeBlacklistedUrl(&blacklist_url))
+    return;  // Local blacklist is empty.
+
+  // Send blacklisting request.
+  GURL url(blacklist_url_prefix_ +
+           net::EscapeQueryParamValue(blacklist_url.spec(), true));
+  IssueRequest(url);
+}
+
+// static
+bool SuggestionsService::IsBlacklistRequest(const net::URLFetcher& request) {
+  std::string blacklist_url_prefix =
+      GetExperimentParam(kSuggestionsFieldTrialURLParam) +
+      GetExperimentParam(kSuggestionsFieldTrialBlacklistSuffixParam);
+  return StartsWithASCII(request.GetOriginalURL().spec(), blacklist_url_prefix,
+                         true);
+}
+
+// static
+bool SuggestionsService::GetBlacklistedUrl(const net::URLFetcher& request,
+                                           GURL* url) {
+  if (!IsBlacklistRequest(request)) return false;
+
+  std::string blacklisted;
+  // TODO(manzagop): don't hardcode the parameter name. DONOTSUBMIT!
+  if (!net::GetValueForKeyInQuery(request.GetOriginalURL(), std::string("url"),
+                                  &blacklisted))
+    return false;
+
+  GURL blacklisted_url(blacklisted);
+  blacklisted_url.Swap(url);
+  return true;
 }
 
 // static
 void SuggestionsService::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   SuggestionsStore::RegisterProfilePrefs(registry);
+  BlacklistStore::RegisterProfilePrefs(registry);
 }
 
 void SuggestionsService::OnRequestTimeout() {
@@ -188,8 +236,8 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
   DCHECK_EQ(pending_request_.get(), source);
 
-  // We no longer need the timeout closure. Delete it whether or not it has run
-  // (if it hasn't, this cancels it).
+  // We no longer need the timeout closure. Delete it whether or not it has run.
+  // If it hasn't, this cancels it.
   pending_timeout_closure_.reset();
 
   // The fetcher will be deleted when the request is handled.
@@ -203,6 +251,7 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
              << net::ErrorToString(request_status.error());
     // Dispatch the cached profile on error.
     ServeFromCache();
+    ScheduleBlacklistUpload(false);
     return;
   }
 
@@ -212,7 +261,9 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
   if (response_code != net::HTTP_OK) {
     // Aggressively clear the store.
     suggestions_store_->ClearSuggestions();
-    DispatchRequestsAndClear(SuggestionsProfile(), &waiting_requestors_);
+    SuggestionsProfile suggestions;
+    FilterAndServe(&suggestions);

[Mathieu, 2014/06/17 14:51:14]

If it's an empty profile, why not keep DispatchRequestsAndClear (i.e. no need to
filter?)

[manzagop (departed), 2014/06/17 15:42:23]

Done.

+    ScheduleBlacklistUpload(false);
     return;
   }
 
@@ -220,6 +271,12 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
       base::TimeTicks::Now() - last_request_started_time_;
   UMA_HISTOGRAM_MEDIUM_TIMES("Suggestions.FetchSuccessLatency", latency);
 
+  // Handle a successful blacklisting.
+  GURL blacklisted_url;
+  if (GetBlacklistedUrl(*source, &blacklisted_url)) {
+    blacklist_store_->RemoveUrl(blacklisted_url);
+  }
+
   std::string suggestions_data;
   bool success = request->GetResponseAsString(&suggestions_data);
   DCHECK(success);
@@ -239,7 +296,8 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
     suggestions_store_->LoadSuggestions(&suggestions);
   }
 
-  DispatchRequestsAndClear(suggestions, &waiting_requestors_);
+  FilterAndServe(&suggestions);
+  ScheduleBlacklistUpload(true);
 }
 
 void SuggestionsService::Shutdown() {
@@ -250,12 +308,6 @@ void SuggestionsService::Shutdown() {
   ServeFromCache();
 }
 
-bool SuggestionsService::IsBlacklistRequest(net::URLFetcher* request) const {
-  DCHECK(request);
-  return StartsWithASCII(request->GetOriginalURL().spec(),
-                         blacklist_url_prefix_, true);
-}
-
 net::URLFetcher* SuggestionsService::CreateSuggestionsRequest(const GURL& url) {
   net::URLFetcher* request =
       net::URLFetcher::Create(0, url, net::URLFetcher::GET, this);
@@ -269,10 +321,45 @@ net::URLFetcher* SuggestionsService::CreateSuggestionsRequest(const GURL& url) {
   return request;
 }
 
+void SuggestionsService::IssueRequest(const GURL& url) {

[Mathieu, 2014/06/17 14:51:14]

very nit: I would have this before OnURLFetchComplete, in a logical order

[manzagop (departed), 2014/06/17 15:42:24]

Done.

+  pending_request_.reset(CreateSuggestionsRequest(url));
+  pending_request_->Start();
+  last_request_started_time_ = base::TimeTicks::Now();
+}
+
 void SuggestionsService::ServeFromCache() {
   SuggestionsProfile suggestions;
   suggestions_store_->LoadSuggestions(&suggestions);
-  DispatchRequestsAndClear(suggestions, &waiting_requestors_);
+  FilterAndServe(&suggestions);
+}
+
+void SuggestionsService::FilterAndServe(SuggestionsProfile* suggestions) {
+  blacklist_store_->FilterSuggestions(suggestions);
+  DispatchRequestsAndClear(*suggestions, &waiting_requestors_);
+}
+
+void SuggestionsService::ScheduleBlacklistUpload(bool last_request_successful) {
+  DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
+
+  // Update the scheduling delay.
+  if (last_request_successful) {
+    blacklist_delay_sec_ = kBlacklistInitialDelaySec;
+  } else {
+    unsigned int candidate_delay =
+        blacklist_delay_sec_ * kBlacklistBackoffMultiplier;
+    if (candidate_delay < kBacklistMaxDelaySec)
+      blacklist_delay_sec_ = candidate_delay;
+  }
+
+  // Schedule a blacklist upload task.
+  GURL blacklist_url;
+  if (blacklist_store_->GetSomeBlacklistedUrl(&blacklist_url)) {

[Mathieu, 2014/06/17 14:51:14]

Again "Some" seems random.

[manzagop (departed), 2014/06/17 15:42:23]

Done.

+    base::Closure blacklist_cb = base::Bind(
+        &SuggestionsService::UploadBlacklist, weak_ptr_factory_.GetWeakPtr());
+    BrowserThread::PostDelayedTask(
+        BrowserThread::UI, FROM_HERE, blacklist_cb,
+        base::TimeDelta::FromSeconds(blacklist_delay_sec_));
+  }
 }
 
 }  // namespace suggestions