[Most Visited] Check for Sync state when using SuggestionsService.

BUG=386454
R=manzagop@chromium.org, newt@chromium.org
TBR=erikwright
TEST=SuggestionsServiceTest

Review URL: https://codereview.chromium.org/473123002

Cr-Commit-Position: refs/heads/master@{#291116}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@291116 0039d316-1c4b-4281-b951-d872f2087c98

chrome/browser/android/most_visited_sites.cc

@@ -24,8 +24,11 @@
 #include "chrome/browser/profiles/profile_android.h"
 #include "chrome/browser/search/suggestions/suggestions_service_factory.h"
 #include "chrome/browser/search/suggestions/suggestions_source.h"
+#include "chrome/browser/sync/profile_sync_service.h"
+#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "chrome/browser/thumbnails/thumbnail_list_source.h"
 #include "components/suggestions/suggestions_service.h"
+#include "components/suggestions/suggestions_utils.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/url_data_source.h"
@@ -46,6 +49,7 @@ using suggestions::ChromeSuggestion;
 using suggestions::SuggestionsProfile;
 using suggestions::SuggestionsService;
 using suggestions::SuggestionsServiceFactory;
+using suggestions::SyncState;
 
 namespace {
 
@@ -171,6 +175,18 @@ void LogHistogramEvent(const std::string& histogram, int position,
     counter->Add(position);
 }
 
+// Return the current SyncState for use with the SuggestionsService.
+SyncState GetSyncState(Profile* profile) {
+  ProfileSyncService* sync =
+      ProfileSyncServiceFactory::GetInstance()->GetForProfile(profile);
+  if (!sync)
+    return SyncState::SYNC_OR_HISTORY_SYNC_DISABLED;
+  return suggestions::GetSyncState(
+      sync->IsSyncEnabledAndLoggedIn(),
+      sync->sync_initialized(),
+      sync->GetActiveDataTypes().Has(syncer::HISTORY_DELETE_DIRECTIVES));
+}
+
 }  // namespace
 
 MostVisitedSites::MostVisitedSites(Profile* profile)
@@ -182,6 +198,14 @@ MostVisitedSites::MostVisitedSites(Profile* profile)
   content::URLDataSource::Add(profile_,
                               new suggestions::SuggestionsSource(profile_));
   content::URLDataSource::Add(profile_, new ThumbnailListSource(profile_));
+
+  // Register this class as an observer to the sync service. It is important to
+  // be notified of changes in the sync state such as initialization, sync
+  // being enabled or disabled, etc.
+  ProfileSyncService* profile_sync_service =
+      ProfileSyncServiceFactory::GetForProfile(profile_);
+  if (profile_sync_service)
+    profile_sync_service->AddObserver(this);
 }
 
 MostVisitedSites::~MostVisitedSites() {
@@ -230,11 +254,13 @@ void MostVisitedSites::GetURLThumbnail(JNIEnv* env,
   std::string url_string = ConvertJavaStringToUTF8(env, url);
   scoped_refptr<TopSites> top_sites(profile_->GetTopSites());
 
-  // If the Suggestions service is enabled, create a callback to fetch a
-  // server thumbnail from it, in case the local thumbnail is not found.
+  // If the Suggestions service is enabled and in use, create a callback to
+  // fetch a server thumbnail from it, in case the local thumbnail is not found.
   SuggestionsService* suggestions_service =
       SuggestionsServiceFactory::GetForProfile(profile_);
-  base::Closure lookup_failed_callback = suggestions_service ?
+  bool use_suggestions_service = suggestions_service &&
+      mv_source_ == SUGGESTIONS_SERVICE;
+  base::Closure lookup_failed_callback = use_suggestions_service ?
       base::Bind(&MostVisitedSites::GetSuggestionsThumbnailOnUIThread,
                  weak_ptr_factory_.GetWeakPtr(),
                  suggestions_service, url_string,
@@ -317,6 +343,13 @@ void MostVisitedSites::Observe(int type,
   }
 }
 
+void MostVisitedSites::OnStateChanged() {
+  // There have been changes to the sync state. This class cares about a few
+  // (just initialized, enabled/disabled or history sync state changed). Re-run
+  // the query code which will use the proper state.
+  QueryMostVisitedURLs();
+}
+
 // static
 bool MostVisitedSites::Register(JNIEnv* env) {
   return RegisterNativesImpl(env);
@@ -328,6 +361,7 @@ void MostVisitedSites::QueryMostVisitedURLs() {
   if (suggestions_service) {
     // Suggestions service is enabled, initiate a query.
     suggestions_service->FetchSuggestionsData(
+        GetSyncState(profile_),
         base::Bind(
           &MostVisitedSites::OnSuggestionsProfileAvailable,
           weak_ptr_factory_.GetWeakPtr(),

chrome/browser/android/most_visited_sites.h

@@ -12,6 +12,7 @@
 #include "base/memory/weak_ptr.h"
 #include "chrome/browser/history/history_types.h"
 #include "chrome/browser/profiles/profile.h"
+#include "chrome/browser/sync/profile_sync_service_observer.h"
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "content/public/browser/notification_observer.h"
 #include "content/public/browser/notification_registrar.h"
@@ -21,7 +22,8 @@ class SuggestionsService;
 }
 
 // Provides the list of most visited sites and their thumbnails to Java.
-class MostVisitedSites : public content::NotificationObserver {
+class MostVisitedSites : public ProfileSyncServiceObserver,
+                         public content::NotificationObserver {
  public:
   typedef base::Callback<
       void(base::android::ScopedJavaGlobalRef<jobject>* bitmap,
@@ -47,6 +49,9 @@ class MostVisitedSites : public content::NotificationObserver {
                        const content::NotificationSource& source,
                        const content::NotificationDetails& details) OVERRIDE;
 
+  // ProfileSyncServiceObserver implementation.
+  virtual void OnStateChanged() OVERRIDE;
+
   // Registers JNI methods.
   static bool Register(JNIEnv* env);
 

chrome/browser/search/suggestions/suggestions_source.cc

@@ -21,6 +21,7 @@
 #include "chrome/browser/search/suggestions/suggestions_service_factory.h"
 #include "chrome/common/url_constants.h"
 #include "components/suggestions/suggestions_service.h"
+#include "components/suggestions/suggestions_utils.h"
 #include "net/base/escape.h"
 #include "ui/base/l10n/time_format.h"
 #include "ui/gfx/codec/png_codec.h"
@@ -126,7 +127,10 @@ void SuggestionsSource::StartDataRequest(
     return;
   }
 
+  // Since it's a debugging page, it's fine to specify that sync state is
+  // initialized.
   suggestions_service->FetchSuggestionsData(
+      INITIALIZED_ENABLED_HISTORY,
       base::Bind(&SuggestionsSource::OnSuggestionsAvailable,
                  weak_ptr_factory_.GetWeakPtr(), callback));
 }

components/suggestions.gypi

@@ -31,6 +31,8 @@
         'suggestions/suggestions_service.h',
         'suggestions/suggestions_store.cc',
         'suggestions/suggestions_store.h',
+        'suggestions/suggestions_utils.cc',
+        'suggestions/suggestions_utils.h',
       ],
       'variables': {
         'proto_in_dir': 'suggestions/proto',

components/suggestions/BUILD.gn

@@ -13,6 +13,8 @@ static_library("suggestions") {
     "suggestions_service.h",
     "suggestions_store.cc",
     "suggestions_store.h",
+    "suggestions_utils.cc",
+    "suggestions_utils.h",
   ]
 
   deps = [

components/suggestions/suggestions_service.cc

@@ -156,8 +156,25 @@ bool SuggestionsService::IsControlGroup() {
 }
 
 void SuggestionsService::FetchSuggestionsData(
+    SyncState sync_state,
     SuggestionsService::ResponseCallback callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  if (sync_state == NOT_INITIALIZED_ENABLED) {
+    // Sync is not initialized yet, but enabled. Serve previously cached
+    // suggestions if available.
+    waiting_requestors_.push_back(callback);
+    ServeFromCache();
+    return;
+  } else if (sync_state == SYNC_OR_HISTORY_SYNC_DISABLED) {
+    // Cancel any ongoing request (and the timeout closure). We must no longer
+    // interact with the server.
+    pending_request_.reset(NULL);
+    pending_timeout_closure_.reset(NULL);
+    suggestions_store_->ClearSuggestions();
+    callback.Run(SuggestionsProfile());
+    DispatchRequestsAndClear(SuggestionsProfile(), &waiting_requestors_);
+    return;
+  }
 
   FetchSuggestionsDataNoTimeout(callback);
 
@@ -170,21 +187,6 @@ void SuggestionsService::FetchSuggestionsData(
       base::TimeDelta::FromMilliseconds(request_timeout_ms_));
 }
 
-void SuggestionsService::FetchSuggestionsDataNoTimeout(
-    SuggestionsService::ResponseCallback callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (pending_request_.get()) {
-    // Request already exists, so just add requestor to queue.
-    waiting_requestors_.push_back(callback);
-    return;
-  }
-
-  // Form new request.
-  DCHECK(waiting_requestors_.empty());
-  waiting_requestors_.push_back(callback);
-  IssueRequest(suggestions_url_);
-}
-
 void SuggestionsService::GetPageThumbnail(
     const GURL& url,
     base::Callback<void(const GURL&, const SkBitmap*)> callback) {
@@ -235,6 +237,33 @@ void SuggestionsService::RegisterProfilePrefs(
   BlacklistStore::RegisterProfilePrefs(registry);
 }
 
+void SuggestionsService::SetDefaultExpiryTimestamp(
+    SuggestionsProfile* suggestions, int64 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);
+    }
+  }
+}
+
+void SuggestionsService::FetchSuggestionsDataNoTimeout(
+    SuggestionsService::ResponseCallback callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  if (pending_request_.get()) {
+    // Request already exists, so just add requestor to queue.
+    waiting_requestors_.push_back(callback);
+    return;
+  }
+
+  // Form new request.
+  DCHECK(waiting_requestors_.empty());
+  waiting_requestors_.push_back(callback);
+  IssueRequest(suggestions_url_);
+}
+
 void SuggestionsService::IssueRequest(const GURL& url) {
   pending_request_.reset(CreateSuggestionsRequest(url));
   pending_request_->Start();
@@ -330,18 +359,6 @@ void SuggestionsService::OnURLFetchComplete(const net::URLFetcher* source) {
   ScheduleBlacklistUpload(true);
 }
 
-void SuggestionsService::SetDefaultExpiryTimestamp(
-    SuggestionsProfile* suggestions, int64 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);
-    }
-  }
-}
-
 void SuggestionsService::Shutdown() {
   // Cancel pending request and timeout closure, then serve existing requestors
   // from cache.

components/suggestions/suggestions_service.h

@@ -19,6 +19,7 @@
 #include "components/keyed_service/core/keyed_service.h"
 #include "components/suggestions/image_manager.h"
 #include "components/suggestions/proto/suggestions.pb.h"
+#include "components/suggestions/suggestions_utils.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "ui/gfx/image/image_skia.h"
 #include "url/gurl.h"
@@ -64,16 +65,20 @@ class SuggestionsService : public KeyedService, public net::URLFetcherDelegate {
   // Whether the user is part of a control group.
   static bool IsControlGroup();
 
-  // Request suggestions data, which will be passed to |callback|. Initiates a
-  // fetch request unless a pending one exists. To prevent multiple requests,
-  // we place all |callback|s in a queue and update them simultaneously when
-  // fetch request completes. Also posts a task to execute OnRequestTimeout
-  // if the request hasn't completed in a given amount of time.
-  void FetchSuggestionsData(ResponseCallback callback);
-
-  // Similar to FetchSuggestionsData but doesn't post a task to execute
-  // OnDelaySinceFetch.
-  void FetchSuggestionsDataNoTimeout(ResponseCallback callback);
+  // Request suggestions data, which will be passed to |callback|. |sync_state|
+  // will influence the behavior of this function (see SyncState definition).
+  //
+  // |sync_state| must be specified based on the current state of the system
+  // (see suggestions::GetSyncState). Callers should call this function again if
+  // sync state changes.
+  //
+  // If state allows for a network request, it is initiated unless a pending one
+  // exists. To prevent multiple requests, all |callback|s are placed in a queue
+  // and are updated simultaneously when the fetch completes. Also posts a task
+  // to execute OnRequestTimeout if the request hasn't completed in a given
+  // amount of time.
+  void FetchSuggestionsData(SyncState sync_state,
+                            ResponseCallback callback);
 
   // Retrieves stored thumbnail for website |url| asynchronously. Calls
   // |callback| with Bitmap pointer if found, and NULL otherwise.
@@ -97,10 +102,15 @@ class SuggestionsService : public KeyedService, public net::URLFetcherDelegate {
   void SetDefaultExpiryTimestamp(SuggestionsProfile* suggestions,
                                  int64 timestamp_usec);
  private:
+  friend class SuggestionsServiceTest;
   FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, BlacklistURLFails);
   FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, FetchSuggestionsData);
   FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, UpdateBlacklistDelay);
 
+  // Similar to FetchSuggestionsData but doesn't post a task to execute
+  // OnDelaySinceFetch.
+  void FetchSuggestionsDataNoTimeout(ResponseCallback callback);
+
   // Issue a request.
   void IssueRequest(const GURL& url);
 

components/suggestions/suggestions_service_unittest.cc

@@ -18,6 +18,7 @@
 #include "components/suggestions/image_manager.h"
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "components/suggestions/suggestions_store.h"
+#include "components/suggestions/suggestions_utils.h"
 #include "components/variations/entropy_provider.h"
 #include "components/variations/variations_associated_data.h"
 #include "net/http/http_response_headers.h"
@@ -326,6 +327,7 @@ TEST_F(SuggestionsServiceTest, FetchSuggestionsDataRequestError) {
 
   // Send the request. Empty data will be returned to the callback.
   suggestions_service->FetchSuggestionsData(
+      INITIALIZED_ENABLED_HISTORY,  // Normal mode.
       base::Bind(&SuggestionsServiceTest::ExpectEmptySuggestionsProfile,
                  base::Unretained(this)));
 
@@ -361,6 +363,7 @@ TEST_F(SuggestionsServiceTest, FetchSuggestionsDataResponseNotOK) {
 
   // Send the request. Empty data will be returned to the callback.
   suggestions_service->FetchSuggestionsData(
+      INITIALIZED_ENABLED_HISTORY,  // Normal mode.
       base::Bind(&SuggestionsServiceTest::ExpectEmptySuggestionsProfile,
                  base::Unretained(this)));
 
@@ -371,6 +374,62 @@ TEST_F(SuggestionsServiceTest, FetchSuggestionsDataResponseNotOK) {
   EXPECT_EQ(1, suggestions_empty_data_count_);
 }
 
+TEST_F(SuggestionsServiceTest, FetchSuggestionsDataSyncDisabled) {
+  // Field trial enabled with a specific suggestions URL.
+  EnableFieldTrial(kFakeSuggestionsURL, kFakeSuggestionsCommonParams,
+                   kFakeBlacklistPath, kFakeBlacklistUrlParam, false);
+  scoped_ptr<SuggestionsService> suggestions_service(
+      CreateSuggestionsServiceWithMocks());
+  EXPECT_TRUE(suggestions_service != NULL);
+
+  // Set up expectations on the SuggestionsStore.
+  EXPECT_CALL(*mock_suggestions_store_, ClearSuggestions());
+
+  // Send the request. Cache is cleared and empty data will be returned to the
+  // callback.
+  suggestions_service->FetchSuggestionsData(
+      SYNC_OR_HISTORY_SYNC_DISABLED,
+      base::Bind(&SuggestionsServiceTest::ExpectEmptySuggestionsProfile,
+                 base::Unretained(this)));
+
+  // Wait for posted task to complete.
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Ensure that ExpectEmptySuggestionsProfile ran once.
+  EXPECT_EQ(1, suggestions_empty_data_count_);
+}
+
+TEST_F(SuggestionsServiceTest, FetchSuggestionsDataSyncNotInitializedEnabled) {
+  // Field trial enabled with a specific suggestions URL.
+  EnableFieldTrial(kFakeSuggestionsURL, kFakeSuggestionsCommonParams,
+                   kFakeBlacklistPath, kFakeBlacklistUrlParam, false);
+  scoped_ptr<SuggestionsService> suggestions_service(
+      CreateSuggestionsServiceWithMocks());
+  EXPECT_TRUE(suggestions_service != NULL);
+  scoped_ptr<SuggestionsProfile> suggestions_profile(
+      CreateSuggestionsProfile());
+
+  // Expectations.
+  EXPECT_CALL(*mock_suggestions_store_, LoadSuggestions(_))
+      .WillOnce(DoAll(SetArgPointee<0>(*suggestions_profile), Return(true)));
+  EXPECT_CALL(*mock_thumbnail_manager_,
+              Initialize(EqualsProto(*suggestions_profile)));
+  EXPECT_CALL(*mock_blacklist_store_, FilterSuggestions(_));
+
+  // Send the request. In this state, cached data will be returned to the
+  // caller.
+  suggestions_service->FetchSuggestionsData(
+      NOT_INITIALIZED_ENABLED,
+      base::Bind(&SuggestionsServiceTest::CheckSuggestionsData,
+                 base::Unretained(this)));
+
+  // Wait for posted task to complete.
+  base::MessageLoop::current()->RunUntilIdle();
+
+  // Ensure that CheckSuggestionsData ran once.
+  EXPECT_EQ(1, suggestions_data_check_count_);
+}
+
 TEST_F(SuggestionsServiceTest, BlacklistURL) {
   EnableFieldTrial(kFakeSuggestionsURL, kFakeSuggestionsCommonParams,
                    kFakeBlacklistPath, kFakeBlacklistUrlParam, false);

components/suggestions/suggestions_utils.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_utils.h"
+
+namespace suggestions {
+
+SyncState GetSyncState(bool sync_enabled,
+                       bool sync_initialized,
+                       bool history_sync_enabled) {
+  if (!sync_enabled)
+    return SYNC_OR_HISTORY_SYNC_DISABLED;
+
+  if (!sync_initialized)
+    return NOT_INITIALIZED_ENABLED;
+
+  return history_sync_enabled ?
+      INITIALIZED_ENABLED_HISTORY : SYNC_OR_HISTORY_SYNC_DISABLED;
+}
+
+}  // namespace suggestions

components/suggestions/suggestions_utils.h

+// 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.
+
+#ifndef COMPONENTS_SUGGESTIONS_SUGGESTIONS_UTILS_H_
+#define COMPONENTS_SUGGESTIONS_SUGGESTIONS_UTILS_H_
+
+namespace suggestions {
+
+// Establishes the different sync states that users of SuggestionsService can
+// specify. 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,
+};
+
+// Users of SuggestionsService should always use this function to get SyncState.
+SyncState GetSyncState(bool sync_enabled,
+                       bool sync_initialized,
+                       bool history_sync_enabled);
+
+}  // namespace suggestions
+
+#endif  // COMPONENTS_SUGGESTIONS_SUGGESTIONS_UTILS_H_