SuggestionsService: don't automatically fetch on startup

components/suggestions/suggestions_service_impl.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_impl.h"
 
 #include <memory>
 #include <utility>
 
 #include "base/feature_list.h"
@@ skipping 162 matching lines @@
     const GURL& url,
     const GURL& thumbnail_url,
     const BitmapCallback& callback) {
   thumbnail_manager_->AddImageURL(url, thumbnail_url);
   GetPageThumbnail(url, callback);
 }
 
 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;
 
   callback_list_.Notify(
       GetSuggestionsDataFromCache().value_or(SuggestionsProfile()));
 
   // 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;
 }
 
 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) &&
       blacklist_store_->RemoveUrl(url)) {
     // The URL was not yet candidate for upload to the server and could be
     // removed from the blacklist.
     callback_list_.Notify(
         GetSuggestionsDataFromCache().value_or(SuggestionsProfile()));
     return true;
   }
   return false;
 }
 
 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()));
   IssueRequestIfNoneOngoing(BuildSuggestionsBlacklistClearURL());
 }
 
 // static
 bool SuggestionsServiceImpl::GetBlacklistedUrl(const net::URLFetcher& request,
                                                GURL* url) {
   bool is_blacklist_request = base::StartsWith(
@@ skipping 56 matching lines @@
   }
   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;
 }
 
-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;
   }
 }
 
 void SuggestionsServiceImpl::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);
     }
   }
 }
 
 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;
   }
   // If there is an ongoing token request, also wait for that.
   if (token_fetcher_) {
     return;
   }
 
   OAuth2TokenService::ScopeSet scopes{GaiaConstants::kChromeSyncOAuth2Scope};
   token_fetcher_ = base::MakeUnique<AccessTokenFetcher>(
@@ skipping 211 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