SuggestionsService undo blacklist functionality.

components/suggestions/suggestions_service.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 "components/suggestions/suggestions_service.h"
 
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop/message_loop_proxy.h"
@@ skipping 12 matching lines @@
 #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"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_status.h"
 #include "url/gurl.h"
 
 using base::CancelableClosure;
+using base::TimeDelta;
+using base::TimeTicks;
 
 namespace suggestions {
 
 namespace {
 
 // Used to UMA log the state of the last response from the server.
 enum SuggestionsResponseState {
   RESPONSE_EMPTY,
   RESPONSE_INVALID,
   RESPONSE_VALID,
@@ skipping 24 matching lines @@
     const SuggestionsProfile& suggestions,
     std::vector<SuggestionsService::ResponseCallback>* requestors) {
   std::vector<SuggestionsService::ResponseCallback> temp_requestors;
   temp_requestors.swap(*requestors);
   std::vector<SuggestionsService::ResponseCallback>::iterator it;
   for (it = temp_requestors.begin(); it != temp_requestors.end(); ++it) {
     if (!it->is_null()) it->Run(suggestions);
   }
 }
 
-// Default delay used when scheduling a blacklist request.
-const int kBlacklistDefaultDelaySec = 1;
+// Default delay used when scheduling a request.
+const TimeDelta kDefaultSchedulingDelay = TimeDelta::FromSeconds(1);

[Mathieu, 2014/12/04 18:53:32]

consider having a const int and rather initialize TimeDelta in the constructor.

[manzagop (departed), 2014/12/05 15:13:22]

Done. Went with local variables.

 
-// Multiplier on the delay used when scheduling a blacklist request, in case the
-// last observed request was unsuccessful.
-const int kBlacklistBackoffMultiplier = 2;
+// Multiplier on the delay used when re-scheduling a failed request.
+const int kSchedulingBackoffMultiplier = 2;
 
 // Maximum valid delay for scheduling a request. Candidate delays larger than
-// this are rejected. This means the maximum backoff is at least 300 / 2, i.e.
-// 2.5 minutes.
-const int kBlacklistMaxDelaySec = 300;  // 5 minutes
+// this are rejected. This means the maximum backoff is at least 5 / 2 minutes.
+const TimeDelta kSchedulingMaxDelaySec = TimeDelta::FromMinutes(5);
 
 }  // namespace
 
 const char kSuggestionsFieldTrialName[] = "ChromeSuggestions";
 const char kSuggestionsFieldTrialControlParam[] = "control";
 const char kSuggestionsFieldTrialStateEnabled[] = "enabled";
 
 // TODO(mathp): Put this in TemplateURL.
 const char kSuggestionsURL[] = "https://www.google.com/chromesuggestions?t=2";
 const char kSuggestionsBlacklistURLPrefix[] =
     "https://www.google.com/chromesuggestions/blacklist?t=2&url=";
 const char kSuggestionsBlacklistURLParam[] = "url";
 
 // The default expiry timeout is 72 hours.
 const int64 kDefaultExpiryUsec = 72 * base::Time::kMicrosecondsPerHour;
 
 SuggestionsService::SuggestionsService(
     net::URLRequestContextGetter* url_request_context,
     scoped_ptr<SuggestionsStore> suggestions_store,
     scoped_ptr<ImageManager> thumbnail_manager,
     scoped_ptr<BlacklistStore> blacklist_store)
     : url_request_context_(url_request_context),
       suggestions_store_(suggestions_store.Pass()),
       thumbnail_manager_(thumbnail_manager.Pass()),
       blacklist_store_(blacklist_store.Pass()),
-      blacklist_delay_sec_(kBlacklistDefaultDelaySec),
+      scheduling_delay_(kDefaultSchedulingDelay),
       suggestions_url_(kSuggestionsURL),
       blacklist_url_prefix_(kSuggestionsBlacklistURLPrefix),
       weak_ptr_factory_(this) {}
 
 SuggestionsService::~SuggestionsService() {}
 
 // static
 bool SuggestionsService::IsControlGroup() {
   return GetExperimentParam(kSuggestionsFieldTrialControlParam) ==
          kSuggestionsFieldTrialStateEnabled;
@@ skipping 23 matching lines @@
 }
 
 void SuggestionsService::GetPageThumbnail(
     const GURL& url,
     base::Callback<void(const GURL&, const SkBitmap*)> callback) {
   thumbnail_manager_->GetImageForURL(url, callback);
 }
 
 void SuggestionsService::BlacklistURL(
     const GURL& candidate_url,
-    const SuggestionsService::ResponseCallback& callback) {
+    const SuggestionsService::ResponseCallback& callback,
+    const base::Closure& fail_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  if (!blacklist_store_->BlacklistUrl(candidate_url)) {
+    fail_callback.Run();
+    return;
+  }
+
   waiting_requestors_.push_back(callback);
+  ServeFromCache();
+  if (!pending_request_.get()) {

[Mathieu, 2014/12/04 18:53:33]

put comment above here, mentioning why we only schedule when there isn't already
a request in progress.

[manzagop (departed), 2014/12/05 15:13:22]

Done.

+    ScheduleBlacklistUpload();
+  }
+}
 
-  // Blacklist locally for immediate effect and serve the requestors.
-  blacklist_store_->BlacklistUrl(candidate_url);
-  ServeFromCache();
-
-  // Send blacklisting request. Even if this request ends up not being sent
-  // because of an ongoing request, a blacklist request is later scheduled.
-  // TODO(mathp): Currently, this will not send a request if there is already
-  // a request in flight (for suggestions or blacklist). Should we prioritize
-  // blacklist requests since they actually carry a payload?
-  IssueRequestIfNoneOngoing(
-      BuildBlacklistRequestURL(blacklist_url_prefix_, candidate_url));
+void SuggestionsService::UndoBlacklistURL(
+    const GURL& url,
+    const SuggestionsService::ResponseCallback& callback,
+    const base::Closure& fail_callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  TimeDelta time_delta;
+  if (blacklist_store_->GetTimeUntilReadyForUpload(url, &time_delta) &&
+      time_delta > TimeDelta::FromSeconds(0) &&
+      blacklist_store_->RemoveUrl(url)) {
+    // The URL was in the blacklist without being ready for upload and was

[Mathieu, 2014/12/04 18:53:32]

// The URL was not yet uploaded to the server and could be removed from the
blacklist.

[manzagop (departed), 2014/12/05 15:13:22]

Changed it, though not quite to what you propose because we can't remove a URL
that hasn't been uploaded if we're past the deadline.

+    // successfully removed.
+    waiting_requestors_.push_back(callback);
+    ServeFromCache();
+    return;
+  }
+  fail_callback.Run();
 }
 
 // static
 bool SuggestionsService::GetBlacklistedUrl(const net::URLFetcher& request,
                                            GURL* url) {
   bool is_blacklist_request = StartsWithASCII(request.GetOriginalURL().spec(),
                                               kSuggestionsBlacklistURLPrefix,
                                               true);
   if (!is_blacklist_request) return false;
 
@@ skipping 30 matching lines @@
   }
 }
 
 void SuggestionsService::IssueRequestIfNoneOngoing(const GURL& url) {
   // If there is an ongoing request, let it complete.
   if (pending_request_.get()) {
     return;
   }
   pending_request_.reset(CreateSuggestionsRequest(url));
   pending_request_->Start();
-  last_request_started_time_ = base::TimeTicks::Now();
+  last_request_started_time_ = TimeTicks::Now();
 }
 
 net::URLFetcher* SuggestionsService::CreateSuggestionsRequest(const GURL& url) {
   net::URLFetcher* request =
       net::URLFetcher::Create(0, url, net::URLFetcher::GET, this);
   request->SetLoadFlags(net::LOAD_DISABLE_CACHE);
   request->SetRequestContext(url_request_context_);
   // Add Chrome experiment state to the request headers.
   net::HttpRequestHeaders headers;
   variations::VariationsHttpHeaderProvider::GetInstance()->AppendHeaders(
@@ skipping 11 matching lines @@
 
   const net::URLRequestStatus& request_status = request->GetStatus();
   if (request_status.status() != net::URLRequestStatus::SUCCESS) {
     // This represents network errors (i.e. the server did not provide a
     // response).
     UMA_HISTOGRAM_SPARSE_SLOWLY("Suggestions.FailedRequestErrorCode",
                                 -request_status.error());
     DVLOG(1) << "Suggestions server request failed with error: "
              << request_status.error() << ": "
              << net::ErrorToString(request_status.error());
-    ScheduleBlacklistUpload(false);
+    UpdateBlacklistDelay(false);
+    ScheduleBlacklistUpload();
     return;
   }
 
   const int response_code = request->GetResponseCode();
   UMA_HISTOGRAM_SPARSE_SLOWLY("Suggestions.FetchResponseCode", response_code);
   if (response_code != net::HTTP_OK) {
     // A non-200 response code means that server has no (longer) suggestions for
     // this user. Aggressively clear the cache.
     suggestions_store_->ClearSuggestions();
-    ScheduleBlacklistUpload(false);
+    UpdateBlacklistDelay(false);
+    ScheduleBlacklistUpload();
     return;
   }
 
-  const base::TimeDelta latency =
-      base::TimeTicks::Now() - last_request_started_time_;
+  const TimeDelta latency = 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);
 
   // Parse the received suggestions and update the cache, or take proper action
   // in the case of invalid response.
   SuggestionsProfile suggestions;
   if (suggestions_data.empty()) {
     LogResponseState(RESPONSE_EMPTY);
     suggestions_store_->ClearSuggestions();
   } else if (suggestions.ParseFromString(suggestions_data)) {
     LogResponseState(RESPONSE_VALID);
     int64 now_usec = (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())
         .ToInternalValue();
     SetDefaultExpiryTimestamp(&suggestions, now_usec + kDefaultExpiryUsec);
     suggestions_store_->StoreSuggestions(suggestions);
   } else {
     LogResponseState(RESPONSE_INVALID);
   }
 
-  ScheduleBlacklistUpload(true);
+  UpdateBlacklistDelay(true);
+  ScheduleBlacklistUpload();
 }
 
 void SuggestionsService::Shutdown() {
   // Cancel pending request, then serve existing requestors from cache.
   pending_request_.reset(NULL);
   ServeFromCache();
 }
 
 void SuggestionsService::ServeFromCache() {
   SuggestionsProfile suggestions;
   // In case of empty cache or error, |suggestions| stays empty.
   suggestions_store_->LoadSuggestions(&suggestions);
   thumbnail_manager_->Initialize(suggestions);
   FilterAndServe(&suggestions);
 }
 
 void SuggestionsService::FilterAndServe(SuggestionsProfile* suggestions) {
   blacklist_store_->FilterSuggestions(suggestions);
   DispatchRequestsAndClear(*suggestions, &waiting_requestors_);
 }
 
-void SuggestionsService::ScheduleBlacklistUpload(bool last_request_successful) {
+void SuggestionsService::ScheduleBlacklistUpload() {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  UpdateBlacklistDelay(last_request_successful);
-
-  // Schedule a blacklist upload task.
-  GURL blacklist_url;
-  if (blacklist_store_->GetFirstUrlFromBlacklist(&blacklist_url)) {
+  TimeDelta time_delta;
+  if (blacklist_store_->GetTimeUntilReadyForUpload(&time_delta)) {
+    // Blacklist cache is not empty: schedule.
     base::Closure blacklist_cb =
         base::Bind(&SuggestionsService::UploadOneFromBlacklist,
                    weak_ptr_factory_.GetWeakPtr());
     base::MessageLoopProxy::current()->PostDelayedTask(
-        FROM_HERE, blacklist_cb,
-        base::TimeDelta::FromSeconds(blacklist_delay_sec_));
+        FROM_HERE, blacklist_cb, time_delta + scheduling_delay_);
   }
 }
 
 void SuggestionsService::UploadOneFromBlacklist() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   GURL blacklist_url;
-  if (!blacklist_store_->GetFirstUrlFromBlacklist(&blacklist_url))
-    return;  // Local blacklist is empty.
+  if (blacklist_store_->GetCandidateForUpload(&blacklist_url)) {
+    // Issue a blacklisting request. Even if this request ends up not being sent
+    // because of an ongoing request, a blacklist request is later scheduled.
+    IssueRequestIfNoneOngoing(
+        BuildBlacklistRequestURL(blacklist_url_prefix_, blacklist_url));
+    return;
+  }
 
-  // Send blacklisting request. Even if this request ends up not being sent
-  // because of an ongoing request, a blacklist request is later scheduled.
-  IssueRequestIfNoneOngoing(
-      BuildBlacklistRequestURL(blacklist_url_prefix_, blacklist_url));
+  // Even though there's no candidate for upload, the blacklist might not be
+  // empty.
+  ScheduleBlacklistUpload();
 }
 
 void SuggestionsService::UpdateBlacklistDelay(bool last_request_successful) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (last_request_successful) {
-    blacklist_delay_sec_ = kBlacklistDefaultDelaySec;
+    scheduling_delay_ = kDefaultSchedulingDelay;
   } else {
-    int candidate_delay = blacklist_delay_sec_ * kBlacklistBackoffMultiplier;
-    if (candidate_delay < kBlacklistMaxDelaySec)
-      blacklist_delay_sec_ = candidate_delay;
+    TimeDelta candidate_delay =
+        scheduling_delay_ * kSchedulingBackoffMultiplier;
+    if (candidate_delay < kSchedulingMaxDelaySec)
+      scheduling_delay_ = candidate_delay;
   }
 }
 
 }  // namespace suggestions