SuggestionsService: don't automatically fetch on startup

components/suggestions/suggestions_service_impl.cc

Index: components/suggestions/suggestions_service_impl.cc
diff --git a/components/suggestions/suggestions_service_impl.cc b/components/suggestions/suggestions_service_impl.cc
index 5a013c8a58481ec29d51c111e457db7cafddd446..27f9d99f797ed4b62ce0a3c9579934bef62a799a 100644
--- a/components/suggestions/suggestions_service_impl.cc
+++ b/components/suggestions/suggestions_service_impl.cc
@@ -180,6 +180,8 @@ void SuggestionsServiceImpl::GetPageThumbnailWithURL(
 bool SuggestionsServiceImpl::BlacklistURL(const GURL& candidate_url) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  // TODO(treib): Do we need to check |sync_state_| here?
+
   if (!blacklist_store_->BlacklistUrl(candidate_url))
     return false;
 
@@ -196,6 +198,9 @@ 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?
+
   TimeDelta time_delta;
   if (blacklist_store_->GetTimeUntilURLReadyForUpload(url, &time_delta) &&
       time_delta > TimeDelta::FromSeconds(0) &&
@@ -211,6 +216,9 @@ bool SuggestionsServiceImpl::UndoBlacklistURL(const GURL& url) {
 
 void SuggestionsServiceImpl::ClearBlacklist() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  // TODO(treib): Do we need to check |sync_state_| here?
+
   blacklist_store_->ClearBlacklist();
   callback_list_.Notify(
       GetSuggestionsDataFromCache().value_or(SuggestionsProfile()));
@@ -287,37 +295,47 @@ SuggestionsServiceImpl::SyncState SuggestionsServiceImpl::ComputeSyncState()
              : SYNC_OR_HISTORY_SYNC_DISABLED;
 }
 
-bool SuggestionsServiceImpl::RefreshSyncState() {
+SuggestionsServiceImpl::RefreshAction
+SuggestionsServiceImpl::RefreshSyncState() {
   SyncState new_sync_state = ComputeSyncState();
   if (sync_state_ == new_sync_state) {
-    return false;
+    return NO_ACTION;
+  }
+
+  RefreshAction result = NO_ACTION;
+  // If the user signed out (or disabled history sync), we have to clear
+  // everything.
+  if (new_sync_state == SYNC_OR_HISTORY_SYNC_DISABLED) {

[mastiz, 2017/05/08 14:18:32]

Optional: How about using a switch with |new_sync_state|? One way would be to
store |old_sync_state|, assign to |sync_state_| early, such that you can use
return statements within the switch (and avoid |result|).

[Marc Treib, 2017/05/08 14:57:46]

Done.

+    result = CLEAR_SUGGESTIONS;
   }
+  // If the user just signed in, we fetch suggestions, so that hopefully the
+  // next NTP will already get them.
+  if (sync_state_ == SYNC_OR_HISTORY_SYNC_DISABLED &&
+      new_sync_state == INITIALIZED_ENABLED_HISTORY) {
+    result = FETCH_SUGGESTIONS;
+  }
+  // Otherwise, there's nothing to do.
+
   sync_state_ = new_sync_state;
-  return true;
+  return result;
 }
 
 void SuggestionsServiceImpl::OnStateChanged(syncer::SyncService* sync) {
   DCHECK(sync_service_ == sync);
 
-  if (!RefreshSyncState()) {
-    return;
-  }
+  RefreshAction action = RefreshSyncState();

[mastiz, 2017/05/08 14:18:32]

Optional: avoid the temporary variable.

[Marc Treib, 2017/05/08 14:57:46]

Done.

 
-  switch (sync_state_) {
-    case SYNC_OR_HISTORY_SYNC_DISABLED:
+  switch (action) {
+    case NO_ACTION:
+      break;
+    case CLEAR_SUGGESTIONS:
       // 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());
+    case FETCH_SUGGESTIONS:
+      IssueRequestIfNoneOngoing(BuildSuggestionsURL());
       break;
   }
 }
@@ -337,6 +355,8 @@ void SuggestionsServiceImpl::SetDefaultExpiryTimestamp(
 
 void SuggestionsServiceImpl::IssueRequestIfNoneOngoing(const GURL& url) {
   // If there is an ongoing request, let it complete.
+  // TODO(treib): This will silently swallow blacklist and clearblacklist
+  // requests if a request happens to be ongoing.

[mastiz, 2017/05/08 14:18:32]

Nit: I'm not a very fan of this TODO phrasing, since there's really no proposed
action. I'd suggest:
// This will silently swallow blacklist and clearblacklist
// requests if a request happens to be ongoing.
// TODO(treib): <Do this or that>.

[Marc Treib, 2017/05/08 14:57:46]

Done.

   if (pending_request_.get()) {
     return;
   }