Fix InstantService not refreshing TopSites when kNtpTilesFeature enabled

InstantService is the only caller of MostVisitedSites::Refresh() and it
is obvious from the caller's side that TopSites should be refreshed too,
when an NTP is opened. Prior to this patch, TopSites would be updated
time-based, leading to more stale NTP tiles when the feature
kNtpTilesFeature is enabled (non-default case).

The bug is believed to make little impact.

Bug: 741431
Change-Id: Iccdc4f89d3ab430eafd9a2faf1b1564f363ff29f
Reviewed-on: https://chromium-review.googlesource.com/581147
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488647}

chrome/browser/ntp_tiles/ntp_tiles_browsertest.cc

@@ -149,9 +149,7 @@ IN_PROC_BROWSER_TEST_F(NTPTilesTest, NavigateAfterSettingObserver) {
       browser(), page_url, WindowOpenDisposition::CURRENT_TAB,
       ui_test_utils::BROWSER_TEST_WAIT_FOR_NAVIGATION);
 
-  // TODO(crbug.com/741431): When Refresh calls SyncWithHistory() for signed
-  // out users, replace the call below with most_visited_sites_->Refresh().
-  most_visited_sites_->top_sites()->SyncWithHistory();
+  most_visited_sites_->Refresh();
   NTPTilesVector tiles = waiter.WaitForTiles();
   EXPECT_THAT(tiles, Contains(MatchesTile("OK", page_url.spec().c_str(),
                                           TileSource::TOP_SITES)));

components/ntp_tiles/most_visited_sites.cc

@@ -125,10 +125,6 @@ void MostVisitedSites::SetMostVisitedURLsObserver(Observer* observer,
   }
 
   if (top_sites_) {
-    // TopSites updates itself after a delay. To ensure up-to-date results,
-    // force an update now.
-    top_sites_->SyncWithHistory();
-
     // Register as TopSitesObserver so that we can update ourselves when the
     // TopSites changes.
     top_sites_observer_.Add(top_sites_.get());
@@ -145,6 +141,14 @@ void MostVisitedSites::SetMostVisitedURLsObserver(Observer* observer,
 }
 
 void MostVisitedSites::Refresh() {
+  if (top_sites_) {
+    // TopSites updates itself after a delay. To ensure up-to-date results,
+    // force an update now.
+    // TODO(mastiz): Is seems unnecessary to refresh TopSites if we will end up
+    // using server-side suggestions.
+    top_sites_->SyncWithHistory();
+  }
+
   suggestions_service_->FetchSuggestionsData();
 }
 

components/ntp_tiles/most_visited_sites_unittest.cc

@@ -444,6 +444,12 @@ TEST_P(MostVisitedSitesTest, ShouldStartNoCallInConstructor) {
   base::RunLoop().RunUntilIdle();
 }
 
+TEST_P(MostVisitedSitesTest, ShouldRefreshBothBackends) {
+  EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
+  EXPECT_CALL(mock_suggestions_service_, FetchSuggestionsData());
+  most_visited_sites_->Refresh();
+}
+
 TEST_P(MostVisitedSitesTest, ShouldIncludeTileForHomePage) {
   FakeHomePageClient* home_page_client = RegisterNewHomePageClient();
   home_page_client->SetHomePageEnabled(true);
@@ -781,7 +787,6 @@ TEST_P(MostVisitedSitesTest, ShouldHandleTopSitesCacheHit) {
           MostVisitedURLList{MakeMostVisitedURL("Site 1", "http://site1/")}));
 
   InSequence seq;
-  EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
   EXPECT_CALL(mock_suggestions_service_, AddCallback(_))
       .WillOnce(Invoke(&suggestions_service_callbacks_,
                        &SuggestionsService::ResponseCallbackList::Add));
@@ -801,6 +806,7 @@ TEST_P(MostVisitedSitesTest, ShouldHandleTopSitesCacheHit) {
                 OnMostVisitedURLsAvailable(ElementsAre(MatchesTile(
                     "Site 1", "http://site1/", TileSource::TOP_SITES))));
   }
+  EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
   EXPECT_CALL(mock_suggestions_service_, FetchSuggestionsData())
       .WillOnce(Return(true));
 
@@ -834,7 +840,6 @@ class MostVisitedSitesWithCacheHitTest : public MostVisitedSitesTest {
   // service has cached results when the observer is registered.
   MostVisitedSitesWithCacheHitTest() {
     InSequence seq;
-    EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
     EXPECT_CALL(mock_suggestions_service_, AddCallback(_))
         .WillOnce(Invoke(&suggestions_service_callbacks_,
                          &SuggestionsService::ResponseCallbackList::Add));
@@ -865,6 +870,7 @@ class MostVisitedSitesWithCacheHitTest : public MostVisitedSitesTest {
                       MatchesTile("Site 3", "http://site3/",
                                   TileSource::SUGGESTIONS_SERVICE))));
     }
+    EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
     EXPECT_CALL(mock_suggestions_service_, FetchSuggestionsData())
         .WillOnce(Return(true));
 
@@ -978,7 +984,6 @@ class MostVisitedSitesWithEmptyCacheTest : public MostVisitedSitesTest {
   // service doesn't have cached results when the observer is registered.
   MostVisitedSitesWithEmptyCacheTest() {
     InSequence seq;
-    EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
     EXPECT_CALL(mock_suggestions_service_, AddCallback(_))
         .WillOnce(Invoke(&suggestions_service_callbacks_,
                          &SuggestionsService::ResponseCallbackList::Add));
@@ -986,6 +991,7 @@ class MostVisitedSitesWithEmptyCacheTest : public MostVisitedSitesTest {
         .WillOnce(Return(SuggestionsProfile()));  // Empty cache.
     EXPECT_CALL(*mock_top_sites_, GetMostVisitedURLs(_, false))
         .WillOnce(Invoke(&top_sites_callbacks_, &TopSitesCallbackList::Add));
+    EXPECT_CALL(*mock_top_sites_, SyncWithHistory());
     EXPECT_CALL(mock_suggestions_service_, FetchSuggestionsData())
         .WillOnce(Return(true));