PopularSites: don't wait for a SuggestionsService network request

chrome/browser/android/ntp/most_visited_sites.cc

 // Copyright 2013 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 "chrome/browser/android/ntp/most_visited_sites.h"
 
 #include <utility>
 
 #include "base/callback.h"
 #include "base/command_line.h"
@@ skipping 164 matching lines @@
 
 MostVisitedSites::Suggestion::Suggestion(Suggestion&&) = default;
 MostVisitedSites::Suggestion&
 MostVisitedSites::Suggestion::operator=(Suggestion&&) = default;
 
 MostVisitedSites::MostVisitedSites(Profile* profile)
     : profile_(profile), top_sites_(TopSitesFactory::GetForProfile(profile)),
       suggestions_service_(SuggestionsServiceFactory::GetForProfile(profile_)),
       observer_(nullptr), num_sites_(0),
       received_most_visited_sites_(false), received_popular_sites_(false),
-      recorded_uma_(false), scoped_observer_(this), weak_ptr_factory_(this) {
+      recorded_uma_(false), scoped_observer_(this),
+      mv_source_(SUGGESTIONS_SERVICE), weak_ptr_factory_(this) {

[Marc Treib, 2016/04/29 13:58:33]

Unrelated, but this used to be uninitialized. Optimistically assuming
SUGGESTIONS_SERVICE (as opposed to TOP_SITES) shouldn't hurt, and is at least
better than random :)

   // Register the thumbnails debugging page.
   content::URLDataSource::Add(profile_, new ThumbnailListSource(profile_));
 
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_);
   supervised_user_service->AddObserver(this);
 }
 
 MostVisitedSites::~MostVisitedSites() {
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile_);
   supervised_user_service->RemoveObserver(this);
 }
 
 void MostVisitedSites::SetMostVisitedURLsObserver(
       MostVisitedSites::Observer* observer, int num_sites) {
+  DCHECK(observer);
   observer_ = observer;
   num_sites_ = num_sites;
 
   if (ShouldShowPopularSites() &&
       NeedPopularSites(profile_->GetPrefs(), num_sites_)) {
     popular_sites_.reset(new PopularSites(
         profile_->GetPrefs(),
         TemplateURLServiceFactory::GetForProfile(profile_),
         profile_->GetRequestContext(),
         GetPopularSitesCountry(),
@@ skipping 12 matching lines @@
 
     // Register as TopSitesObserver so that we can update ourselves when the
     // TopSites changes.
     scoped_observer_.Add(top_sites_.get());
   }
 
   suggestions_subscription_ = suggestions_service_->AddCallback(
       base::Bind(&MostVisitedSites::OnSuggestionsProfileAvailable,
                  base::Unretained(this)));
 
-  // Immediately get the current suggestions from the cache. If the cache is
-  // empty, this will fall back to TopSites.
-  OnSuggestionsProfileAvailable(
-      suggestions_service_->GetSuggestionsDataFromCache());
+  // Immediately build the current suggestions, getting personal suggestions
+  // from the SuggestionsService's cache or, if that is empty, from TopSites.
+  BuildCurrentSuggestions();
   // Also start a request for fresh suggestions.
   suggestions_service_->FetchSuggestionsData();
 }
 
 void MostVisitedSites::GetURLThumbnail(
     const GURL& url,
     const ThumbnailCallback& callback) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   BrowserThread::PostTaskAndReplyWithResult(
@@ skipping 93 matching lines @@
       GetSourceHistogramName(current_suggestions_[index]).c_str());
   LogHistogramEvent(histogram, index, num_sites_);
 
   histogram = base::StringPrintf(
       "NewTabPage.TileTypeClicked.%s",
       GetSourceHistogramName(current_suggestions_[index]).c_str());
   LogHistogramEvent(histogram, tile_type, NUM_TILE_TYPES);
 }
 
 void MostVisitedSites::OnURLFilterChanged() {
-  QueryMostVisitedURLs();
+  BuildCurrentSuggestions();
 }
 
 // static
 void MostVisitedSites::RegisterProfilePrefs(
     user_prefs::PrefRegistrySyncable* registry) {
   registry->RegisterListPref(ntp_tiles::prefs::kNTPSuggestionsURL);
   registry->RegisterListPref(ntp_tiles::prefs::kNTPSuggestionsIsPersonal);
 }
 
-void MostVisitedSites::QueryMostVisitedURLs() {
-  if (suggestions_service_->FetchSuggestionsData()) {
-    // A suggestions network request is on its way. We'll be called back via
-    // OnSuggestionsProfileAvailable.
-    return;

[Marc Treib, 2016/04/29 13:58:33]

This was the actual problem - we'd wait on the network request.
In fact, there's no reason to even send a network request at this point, so I've
just removed it.

-  }
-  // If no network request could be sent, try to get suggestions from the
-  // cache. If that also returns nothing, OnSuggestionsProfileAvailable will
-  // call InitiateTopSitesQuery.
+void MostVisitedSites::BuildCurrentSuggestions() {
+  // Get the current suggestions from cache. If the cache is empty, this will
+  // fall back to TopSites.
   OnSuggestionsProfileAvailable(
       suggestions_service_->GetSuggestionsDataFromCache());
 }
 
 void MostVisitedSites::InitiateTopSitesQuery() {
   if (!top_sites_)
     return;
 
   top_sites_->GetMostVisitedURLs(
       base::Bind(&MostVisitedSites::OnMostVisitedURLsAvailable,
@@ skipping 43 matching lines @@
 
   received_most_visited_sites_ = true;
   mv_source_ = TOP_SITES;
   SaveNewNTPSuggestions(&suggestions);
   NotifyMostVisitedURLsObserver();
 }
 
 void MostVisitedSites::OnSuggestionsProfileAvailable(
     const SuggestionsProfile& suggestions_profile) {
   int num_tiles = suggestions_profile.suggestions_size();
-  // With no server suggestions, fall back to local Most Visited.
+  // With no server suggestions, fall back to local TopSites.
   if (num_tiles == 0) {
     InitiateTopSitesQuery();
     return;
   }
   if (num_sites_ < num_tiles)
     num_tiles = num_sites_;
 
   SupervisedUserURLFilter* url_filter =
       SupervisedUserServiceFactory::GetForProfile(profile_)
           ->GetURLFilterForUIThread();
@@ skipping 279 matching lines @@
   size_t num_dests = dst_suggestions->size();
 
   size_t src_pos = 0;
   size_t i = start_position;
   for (; i < num_dests && src_pos < num_inserts; ++i) {
     if ((*dst_suggestions)[i] != nullptr)
       continue;
     size_t src = insert_positions[src_pos++];
     std::swap((*dst_suggestions)[i], (*src_suggestions)[src]);
   }
-  // Return destination postions filled so far which becomes the start_position
+  // Return destination positions filled so far which becomes the start_position
   // for future runs.
   return i;
 }
 
 void MostVisitedSites::NotifyMostVisitedURLsObserver() {
   size_t num_suggestions = current_suggestions_.size();
   if (received_most_visited_sites_ && received_popular_sites_ &&
       !recorded_uma_) {
     RecordImpressionUMAMetrics();
     UMA_HISTOGRAM_SPARSE_SLOWLY("NewTabPage.NumberOfTiles", num_suggestions);
     recorded_uma_ = true;
   }
 
   if (!observer_)
     return;
 
   observer_->OnMostVisitedURLsAvailable(current_suggestions_);
 }
 
 void MostVisitedSites::OnPopularSitesAvailable(bool success) {
   received_popular_sites_ = true;
 
   if (!success) {
     LOG(WARNING) << "Download of popular sites failed";
     return;
   }
 
-  if (!observer_)

[Marc Treib, 2016/04/29 13:58:33]

|observer_| can't be null here, since we only request the PopularSites once an
observer is set.

-    return;
+  // Pass the popular sites to the observer. This will cause it to fetch any
+  // missing icons, but will *not* cause it to display the popular sites.
+  observer_->OnPopularURLsAvailable(popular_sites_->sites());
 
-  observer_->OnPopularURLsAvailable(popular_sites_->sites());
-  QueryMostVisitedURLs();
+  // Re-build the suggestions list. Once done, this will notify the observer.
+  BuildCurrentSuggestions();
 }
 
 void MostVisitedSites::RecordImpressionUMAMetrics() {
   for (size_t i = 0; i < current_suggestions_.size(); i++) {
     std::string histogram = base::StringPrintf(
         "NewTabPage.SuggestionsImpression.%s",
         GetSourceHistogramName(current_suggestions_[i]).c_str());
     LogHistogramEvent(histogram, static_cast<int>(i), num_sites_);
   }
 }
 
 void MostVisitedSites::TopSitesLoaded(TopSites* top_sites) {}
 
 void MostVisitedSites::TopSitesChanged(TopSites* top_sites,
                                        ChangeReason change_reason) {
   if (mv_source_ == TOP_SITES) {
     // The displayed suggestions are invalidated.
     InitiateTopSitesQuery();
   }
 }