SuggestionsService: Use syncer::GetUploadToGoogleState

instead of hand-rolling its own version of it.

As a convenience for callers, GetUploadToGoogleState now also handles a
null SyncService (which maps to NOT_ACTIVE).

Bug: 824723
Change-Id: Ie81ddc0350ef128ab065c944ecb493f1b1a899ae
Reviewed-on: https://chromium-review.googlesource.com/975132
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#545091}

components/suggestions/suggestions_service_impl.cc

@@ -127,7 +127,7 @@ SuggestionsServiceImpl::SuggestionsServiceImpl(
     : identity_manager_(identity_manager),
       sync_service_(sync_service),
       sync_service_observer_(this),
-      sync_state_(INITIALIZED_ENABLED_HISTORY),
+      history_sync_state_(syncer::UploadState::INITIALIZING),
       url_request_context_(url_request_context),
       suggestions_store_(std::move(suggestions_store)),
       thumbnail_manager_(std::move(thumbnail_manager)),
@@ -152,7 +152,7 @@ SuggestionsServiceImpl::~SuggestionsServiceImpl() {}
 bool SuggestionsServiceImpl::FetchSuggestionsData() {
   DCHECK(thread_checker_.CalledOnValidThread());
   // If sync state allows, issue a network request to refresh the suggestions.
-  if (sync_state_ != INITIALIZED_ENABLED_HISTORY) {
+  if (history_sync_state_ != syncer::UploadState::ACTIVE) {
     return false;
   }
   IssueRequestIfNoneOngoing(BuildSuggestionsURL());
@@ -192,7 +192,7 @@ void SuggestionsServiceImpl::GetPageThumbnailWithURL(
 bool SuggestionsServiceImpl::BlacklistURL(const GURL& candidate_url) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // TODO(treib): Do we need to check |sync_state_| here?
+  // TODO(treib): Do we need to check |history_sync_state_| here?
 
   if (!blacklist_store_->BlacklistUrl(candidate_url))
     return false;
@@ -211,7 +211,7 @@ bool SuggestionsServiceImpl::BlacklistURL(const GURL& candidate_url) {
 bool SuggestionsServiceImpl::UndoBlacklistURL(const GURL& url) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // TODO(treib): Do we need to check |sync_state_| here?
+  // TODO(treib): Do we need to check |history_sync_state_| here?
 
   TimeDelta time_delta;
   if (blacklist_store_->GetTimeUntilURLReadyForUpload(url, &time_delta) &&
@@ -229,7 +229,7 @@ bool SuggestionsServiceImpl::UndoBlacklistURL(const GURL& url) {
 void SuggestionsServiceImpl::ClearBlacklist() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  // TODO(treib): Do we need to check |sync_state_| here?
+  // TODO(treib): Do we need to check |history_sync_state_| here?
 
   blacklist_store_->ClearBlacklist();
   callback_list_.Notify(
@@ -308,55 +308,41 @@ GURL SuggestionsServiceImpl::BuildSuggestionsBlacklistClearURL() {
                                  kDeviceType));
 }
 
-SuggestionsServiceImpl::SyncState SuggestionsServiceImpl::ComputeSyncState()
-    const {
-  if (!sync_service_ || !sync_service_->CanSyncStart() ||
-      sync_service_->IsLocalSyncEnabled() ||
-      sync_service_->GetAuthError().IsPersistentError()) {
-    return SYNC_OR_HISTORY_SYNC_DISABLED;
-  }
-  if (!sync_service_->IsSyncActive() || !sync_service_->ConfigurationDone()) {
-    return NOT_INITIALIZED_ENABLED;
-  }
-  return sync_service_->GetActiveDataTypes().Has(
-             syncer::HISTORY_DELETE_DIRECTIVES)
-             ? INITIALIZED_ENABLED_HISTORY
-             : SYNC_OR_HISTORY_SYNC_DISABLED;
-}
-
 SuggestionsServiceImpl::RefreshAction
-SuggestionsServiceImpl::RefreshSyncState() {
-  SyncState new_sync_state = ComputeSyncState();
-  if (sync_state_ == new_sync_state) {
+SuggestionsServiceImpl::RefreshHistorySyncState() {
+  syncer::UploadState new_sync_state = syncer::GetUploadToGoogleState(
+      sync_service_, syncer::HISTORY_DELETE_DIRECTIVES);
+  if (history_sync_state_ == new_sync_state)
     return NO_ACTION;
-  }
 
-  SyncState old_sync_state = sync_state_;
-  sync_state_ = new_sync_state;
+  syncer::UploadState old_sync_state = history_sync_state_;
+  history_sync_state_ = new_sync_state;
 
   switch (new_sync_state) {
-    case NOT_INITIALIZED_ENABLED:
-      break;
-    case INITIALIZED_ENABLED_HISTORY:
-      // If the user just signed in, we fetch suggestions, so that hopefully the
-      // next NTP will already get them.
-      if (old_sync_state == SYNC_OR_HISTORY_SYNC_DISABLED) {
+    case syncer::UploadState::INITIALIZING:
+      // In this state, we do not issue server requests, but we will serve from
+      // cache if available -> no action required.
+      return NO_ACTION;
+    case syncer::UploadState::ACTIVE:
+      // If history sync was just enabled, immediately fetch suggestions, so
+      // that hopefully the next NTP will already get them.
+      if (old_sync_state == syncer::UploadState::NOT_ACTIVE)
         return FETCH_SUGGESTIONS;
-      }
-      break;
-    case SYNC_OR_HISTORY_SYNC_DISABLED:
+      // Otherwise, this just means sync initialization finished.
+      return NO_ACTION;
+    case syncer::UploadState::NOT_ACTIVE:
       // If the user signed out (or disabled history sync), we have to clear
       // everything.
       return CLEAR_SUGGESTIONS;
   }
-  // Otherwise, there's nothing to do.
+  NOTREACHED();
   return NO_ACTION;
 }
 
 void SuggestionsServiceImpl::OnStateChanged(syncer::SyncService* sync) {
   DCHECK(sync_service_ == sync);
 
-  switch (RefreshSyncState()) {
+  switch (RefreshHistorySyncState()) {
     case NO_ACTION:
       break;
     case CLEAR_SUGGESTIONS:

components/suggestions/suggestions_service_impl.h

@@ -24,6 +24,7 @@
 #include "components/suggestions/proto/suggestions.pb.h"
 #include "components/suggestions/suggestions_service.h"
 #include "components/sync/driver/sync_service_observer.h"
+#include "components/sync/driver/sync_service_utils.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 #include "net/base/backoff_entry.h"
 #include "net/url_request/url_fetcher_delegate.h"
@@ -111,7 +112,8 @@ class SuggestionsServiceImpl : public SuggestionsService,
     SYNC_OR_HISTORY_SYNC_DISABLED,
   };
 
-  // The action that should be taken as the result of a RefreshSyncState call.
+  // The action that should be taken as the result of a RefreshHistorySyncState
+  // call.
   enum RefreshAction { NO_ACTION, FETCH_SUGGESTIONS, CLEAR_SUGGESTIONS };
 
   // Helpers to build the various suggestions URLs. These are static members
@@ -122,12 +124,9 @@ class SuggestionsServiceImpl : public SuggestionsService,
   static GURL BuildSuggestionsBlacklistURL(const GURL& candidate_url);
   static GURL BuildSuggestionsBlacklistClearURL();
 
-  // Computes the appropriate SyncState from |sync_service_|.
-  SyncState ComputeSyncState() const;
-
-  // Re-computes |sync_state_| from the sync service. Returns the action that
-  // should be taken in response.
-  RefreshAction RefreshSyncState() WARN_UNUSED_RESULT;
+  // Re-computes |history_sync_state_| from the sync service. Returns the action
+  // that should be taken in response.
+  RefreshAction RefreshHistorySyncState() WARN_UNUSED_RESULT;
 
   // syncer::SyncServiceObserver implementation.
   void OnStateChanged(syncer::SyncService* sync) override;
@@ -182,7 +181,8 @@ class SuggestionsServiceImpl : public SuggestionsService,
   ScopedObserver<syncer::SyncService, syncer::SyncServiceObserver>
       sync_service_observer_;
 
-  SyncState sync_state_;
+  // The state of history sync, i.e. are we uploading history data to Google?
+  syncer::UploadState history_sync_state_;
 
   net::URLRequestContextGetter* url_request_context_;
 

components/suggestions/suggestions_service_impl_unittest.cc

@@ -87,6 +87,9 @@ class MockSyncService : public syncer::FakeSyncService {
   MOCK_CONST_METHOD0(CanSyncStart, bool());
   MOCK_CONST_METHOD0(IsSyncActive, bool());
   MOCK_CONST_METHOD0(ConfigurationDone, bool());
+  MOCK_CONST_METHOD0(IsLocalSyncEnabled, bool());
+  MOCK_CONST_METHOD0(IsUsingSecondaryPassphrase, bool());
+  MOCK_CONST_METHOD0(GetPreferredDataTypes, syncer::ModelTypeSet());
   MOCK_CONST_METHOD0(GetActiveDataTypes, syncer::ModelTypeSet());
 };
 
@@ -157,6 +160,16 @@ class SuggestionsServiceTest : public testing::Test {
     EXPECT_CALL(*sync_service(), ConfigurationDone())
         .Times(AnyNumber())
         .WillRepeatedly(Return(true));
+    EXPECT_CALL(*sync_service(), IsLocalSyncEnabled())
+        .Times(AnyNumber())
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*sync_service(), IsUsingSecondaryPassphrase())
+        .Times(AnyNumber())
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*sync_service(), GetPreferredDataTypes())
+        .Times(AnyNumber())
+        .WillRepeatedly(
+            Return(syncer::ModelTypeSet(syncer::HISTORY_DELETE_DIRECTIVES)));
     EXPECT_CALL(*sync_service(), GetActiveDataTypes())
         .Times(AnyNumber())
         .WillRepeatedly(

components/sync/driver/sync_service_utils.cc

@@ -23,7 +23,8 @@ UploadState GetUploadToGoogleState(const SyncService* sync_service,
   // Note: Before configuration is done, GetPreferredDataTypes returns
   // "everything" (i.e. the default setting). If a data type is missing there,
   // it must be because the user explicitly disabled it.
-  if (!sync_service->CanSyncStart() || sync_service->IsLocalSyncEnabled() ||
+  if (!sync_service || !sync_service->CanSyncStart() ||
+      sync_service->IsLocalSyncEnabled() ||
       !sync_service->GetPreferredDataTypes().Has(type) ||
       sync_service->GetAuthError().IsPersistentError() ||
       sync_service->IsUsingSecondaryPassphrase()) {