SuggestionsService: implement SyncServiceObserver

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 <utility>
 
 #include "base/feature_list.h"
 #include "base/location.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/metrics/sparse_histogram.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "components/google/core/browser/google_util.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/signin/core/browser/signin_manager_base.h"
 #include "components/suggestions/blacklist_store.h"
 #include "components/suggestions/image_manager.h"
 #include "components/suggestions/suggestions_store.h"
+#include "components/sync_driver/sync_service.h"
 #include "components/variations/net/variations_http_headers.h"
 #include "google_apis/gaia/gaia_constants.h"
 #include "google_apis/gaia/oauth2_token_service.h"
 #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"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_status.h"
 
 using base::TimeDelta;
 using base::TimeTicks;
 
 namespace suggestions {
 
 namespace {
 
+// Establishes the different sync states that matter to SuggestionsService.
+// There are three different concepts in the sync service: initialized, sync
+// enabled and history sync enabled.
+enum SyncState {
+  // State: Sync service is not initialized, yet not disabled. History sync
+  //     state is unknown (since not initialized).
+  // Behavior: Does not issue a server request, but serves from cache if
+  //     available.
+  NOT_INITIALIZED_ENABLED,
+
+  // State: Sync service is initialized, sync is enabled and history sync is
+  //     enabled.
+  // Behavior: Update suggestions from the server. Serve from cache on timeout.
+  INITIALIZED_ENABLED_HISTORY,
+
+  // State: Sync service is disabled or history sync is disabled.
+  // Behavior: Do not issue a server request. Clear the cache. Serve empty
+  //     suggestions.
+  SYNC_OR_HISTORY_SYNC_DISABLED,
+};
+
+SyncState GetSyncState(sync_driver::SyncService* sync) {
+  if (!sync || !sync->CanSyncStart())
+    return SYNC_OR_HISTORY_SYNC_DISABLED;
+  if (!sync->IsSyncActive() || !sync->ConfigurationDone())
+    return NOT_INITIALIZED_ENABLED;
+  return sync->GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES)
+             ? INITIALIZED_ENABLED_HISTORY
+             : SYNC_OR_HISTORY_SYNC_DISABLED;
+}
+
 // Used to UMA log the state of the last response from the server.
 enum SuggestionsResponseState {
   RESPONSE_EMPTY,
   RESPONSE_INVALID,
   RESPONSE_VALID,
   RESPONSE_STATE_SIZE
 };
 
 // Will log the supplied response |state|.
 void LogResponseState(SuggestionsResponseState state) {
@@ skipping 105 matching lines @@
   const SigninManagerBase* signin_manager_;
   OAuth2TokenService* token_service_;
 
   TokenCallback callback_;
   scoped_ptr<OAuth2TokenService::Request> token_request_;
 };
 
 SuggestionsService::SuggestionsService(
     const SigninManagerBase* signin_manager,
     OAuth2TokenService* token_service,
+    sync_driver::SyncService* sync_service,
     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),
+    : sync_service_(sync_service),
+      sync_service_observer_(this),
+      url_request_context_(url_request_context),
       suggestions_store_(std::move(suggestions_store)),
       thumbnail_manager_(std::move(thumbnail_manager)),
       blacklist_store_(std::move(blacklist_store)),
       scheduling_delay_(TimeDelta::FromSeconds(kDefaultSchedulingDelaySec)),
       token_fetcher_(new AccessTokenFetcher(signin_manager, token_service)),
-      weak_ptr_factory_(this) {}
+      weak_ptr_factory_(this) {
+  // |sync_service_| is null if switches::kDisableSync is set (tests use that).
+  if (sync_service_)
+    sync_service_observer_.Add(sync_service_);
+  // Immediately get the current sync state, so we'll flush the cache if
+  // necessary.
+  OnStateChanged();
+}
 
 SuggestionsService::~SuggestionsService() {}
 
-void SuggestionsService::FetchSuggestionsData(
-    SyncState sync_state,
-    const ResponseCallback& callback) {
+bool SuggestionsService::FetchSuggestionsData() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  switch (sync_state) {
-    case SYNC_OR_HISTORY_SYNC_DISABLED:
-      // Cancel any ongoing request, to stop interacting with the server.
-      pending_request_.reset(nullptr);
-      suggestions_store_->ClearSuggestions();
-      if (!callback.is_null())
-        callback.Run(SuggestionsProfile());
-      break;
-    case INITIALIZED_ENABLED_HISTORY:
-    case NOT_INITIALIZED_ENABLED:
-      // TODO(treib): For NOT_INITIALIZED_ENABLED, we shouldn't issue a network
-      // request. Verify that that won't break anything.
-      // Sync is enabled. Serve previously cached suggestions if available, else
-      // an empty set of suggestions.
-      ServeFromCache(callback);
+  // If sync state allows, issue a network request to refresh the suggestions.
+  if (GetSyncState(sync_service_) != INITIALIZED_ENABLED_HISTORY)
+    return false;
+  IssueRequestIfNoneOngoing(BuildSuggestionsURL());
+  return true;
+}
 
-      // Issue a network request to refresh the suggestions in the cache.
-      IssueRequestIfNoneOngoing(BuildSuggestionsURL());
-      break;
-  }
+SuggestionsProfile SuggestionsService::GetSuggestionsDataFromCache() const {
+  SuggestionsProfile suggestions;
+  // In case of empty cache or error, |suggestions| stays empty.
+  suggestions_store_->LoadSuggestions(&suggestions);
+  thumbnail_manager_->Initialize(suggestions);
+  blacklist_store_->FilterSuggestions(&suggestions);
+  return suggestions;
+}
+
+scoped_ptr<SuggestionsService::ResponseCallbackList::Subscription>
+SuggestionsService::AddCallback(const ResponseCallback& callback) {
+  return callback_list_.Add(callback);
 }
 
 void SuggestionsService::GetPageThumbnail(const GURL& url,
                                           const BitmapCallback& callback) {
   thumbnail_manager_->GetImageForURL(url, callback);
 }
 
 void SuggestionsService::GetPageThumbnailWithURL(
     const GURL& url,
     const GURL& thumbnail_url,
     const BitmapCallback& callback) {
   thumbnail_manager_->AddImageURL(url, thumbnail_url);
   GetPageThumbnail(url, callback);
 }
 
-void SuggestionsService::BlacklistURL(const GURL& candidate_url,
-                                      const ResponseCallback& callback,
-                                      const base::Closure& fail_callback) {
+bool SuggestionsService::BlacklistURL(const GURL& candidate_url) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  if (!blacklist_store_->BlacklistUrl(candidate_url)) {
-    if (!fail_callback.is_null())
-      fail_callback.Run();
-    return;
-  }
+  if (!blacklist_store_->BlacklistUrl(candidate_url))
+    return false;
 
-  ServeFromCache(callback);
+  callback_list_.Notify(GetSuggestionsDataFromCache());
+
   // Blacklist uploads are scheduled on any request completion, so only schedule
   // an upload if there is no ongoing request.
   if (!pending_request_.get())
     ScheduleBlacklistUpload();
+
+  return true;
 }
 
-void SuggestionsService::UndoBlacklistURL(const GURL& url,
-                                          const ResponseCallback& callback,
-                                          const base::Closure& fail_callback) {
+bool SuggestionsService::UndoBlacklistURL(const GURL& url) {
   DCHECK(thread_checker_.CalledOnValidThread());
   TimeDelta time_delta;
   if (blacklist_store_->GetTimeUntilURLReadyForUpload(url, &time_delta) &&
       time_delta > TimeDelta::FromSeconds(0) &&
       blacklist_store_->RemoveUrl(url)) {
     // The URL was not yet candidate for upload to the server and could be
     // removed from the blacklist.
-    ServeFromCache(callback);
-    return;
+    callback_list_.Notify(GetSuggestionsDataFromCache());
+    return true;
   }
-  if (!fail_callback.is_null())
-    fail_callback.Run();
+  return false;
 }
 
-void SuggestionsService::ClearBlacklist(const ResponseCallback& callback) {
+void SuggestionsService::ClearBlacklist() {
   DCHECK(thread_checker_.CalledOnValidThread());
   blacklist_store_->ClearBlacklist();
+  callback_list_.Notify(GetSuggestionsDataFromCache());
   IssueRequestIfNoneOngoing(BuildSuggestionsBlacklistClearURL());
-  ServeFromCache(callback);
 }
 
 // static
 bool SuggestionsService::GetBlacklistedUrl(const net::URLFetcher& request,
                                            GURL* url) {
   bool is_blacklist_request = base::StartsWith(
       request.GetOriginalURL().spec(), BuildSuggestionsBlacklistURLPrefix(),
       base::CompareCase::SENSITIVE);
   if (!is_blacklist_request) return false;
 
@@ skipping 43 matching lines @@
               net::EscapeQueryParamValue(candidate_url.spec(), true));
 }
 
 // static
 GURL SuggestionsService::BuildSuggestionsBlacklistClearURL() {
   return GURL(base::StringPrintf(kSuggestionsBlacklistClearURLFormat,
                                  GetGoogleBaseURL().spec().c_str(),
                                  kDeviceType));
 }
 
+void SuggestionsService::OnStateChanged() {
+  switch (GetSyncState(sync_service_)) {
+    case SYNC_OR_HISTORY_SYNC_DISABLED:
+      // Cancel any ongoing request, to stop interacting with the server.
+      pending_request_.reset(nullptr);
+      suggestions_store_->ClearSuggestions();
+      callback_list_.Notify(SuggestionsProfile());
+      break;
+    case NOT_INITIALIZED_ENABLED:
+      // Keep the cache (if any), but don't refresh.
+      break;
+    case INITIALIZED_ENABLED_HISTORY:
+      // If we have any observers, issue a network request to refresh the
+      // suggestions in the cache.
+      if (!callback_list_.empty())
+        IssueRequestIfNoneOngoing(BuildSuggestionsURL());

[Mathieu, 2016/03/08 14:52:34]

So the first OnStateChanged that is called in the constructor of this class will
always have |callback_list_| empty, so no network request. If there are no
future sync state changes, this may never be called in the lifetime of the
browser? Aren't we worried about stale suggestions?

This is why all calls to get suggestions from cache were accompanied with a
network request. To make sure the next caller would have the updated data. Up to
you if you'd like to change it.

[Marc Treib, 2016/03/08 15:31:17]

The main reason I put this in was to avoid lots of calls to /chromesuggestions
that aren't needed, because the SuggestionsService isn't actually used (ie on
desktop so far). Just "on every Chrome startup" amounts to thousands of QPS,
which triggers all sorts of alarms (speaking from personal experience here :D).
IMO when a caller wants fresh data, they should request it. Note that in the
previous system, you'd also get stale data, and there wasn't even a way to wait
for the new data - you'd just have to hope that next time it will be fresher.

+      break;
+  }
+}
+
 void SuggestionsService::SetDefaultExpiryTimestamp(
     SuggestionsProfile* suggestions,
     int64_t default_timestamp_usec) {
   for (int i = 0; i < suggestions->suggestions_size(); ++i) {
     ChromeSuggestion* suggestion = suggestions->mutable_suggestions(i);
     // Do not set expiry if the server has already provided a more specific
     // expiry time for this suggestion.
     if (!suggestion->has_expiry_ts()) {
       suggestion->set_expiry_ts(default_timestamp_usec);
     }
@@ skipping 112 matching lines @@
     int64_t now_usec =
         (base::Time::NowFromSystemTime() - base::Time::UnixEpoch())
             .ToInternalValue();
     SetDefaultExpiryTimestamp(&suggestions, now_usec + kDefaultExpiryUsec);
     PopulateExtraData(&suggestions);
     suggestions_store_->StoreSuggestions(suggestions);
   } else {
     LogResponseState(RESPONSE_INVALID);
   }
 
+  callback_list_.Notify(GetSuggestionsDataFromCache());
+
   UpdateBlacklistDelay(true);
   ScheduleBlacklistUpload();
 }
 
 void SuggestionsService::PopulateExtraData(SuggestionsProfile* suggestions) {
   for (int i = 0; i < suggestions->suggestions_size(); ++i) {
     suggestions::ChromeSuggestion* s = suggestions->mutable_suggestions(i);
     if (!s->has_favicon_url() || s->favicon_url().empty()) {
       s->set_favicon_url(base::StringPrintf(kFaviconURL, s->url().c_str()));
     }
     if (!s->has_impression_url() || s->impression_url().empty()) {
       s->set_impression_url(
           base::StringPrintf(
               kPingURL, static_cast<long long>(suggestions->timestamp()), -1));
     }
 
     if (!s->has_click_url() || s->click_url().empty()) {
       s->set_click_url(base::StringPrintf(
           kPingURL, static_cast<long long>(suggestions->timestamp()), i));
     }
   }
 }
 
 void SuggestionsService::Shutdown() {
   // Cancel pending request.
   pending_request_.reset(nullptr);
 }
 
-void SuggestionsService::ServeFromCache(const ResponseCallback& callback) {
-  SuggestionsProfile suggestions;
-  // In case of empty cache or error, |suggestions| stays empty.
-  suggestions_store_->LoadSuggestions(&suggestions);
-  thumbnail_manager_->Initialize(suggestions);
-  blacklist_store_->FilterSuggestions(&suggestions);
-  if (!callback.is_null())
-    callback.Run(suggestions);
-}
-
 void SuggestionsService::ScheduleBlacklistUpload() {
   DCHECK(thread_checker_.CalledOnValidThread());
   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::ThreadTaskRunnerHandle::Get()->PostDelayedTask(
         FROM_HERE, blacklist_cb, time_delta + scheduling_delay_);
@@ skipping 23 matching lines @@
     scheduling_delay_ = TimeDelta::FromSeconds(kDefaultSchedulingDelaySec);
   } else {
     TimeDelta candidate_delay =
         scheduling_delay_ * kSchedulingBackoffMultiplier;
     if (candidate_delay < TimeDelta::FromSeconds(kSchedulingMaxDelaySec))
       scheduling_delay_ = candidate_delay;
   }
 }
 
 }  // namespace suggestions