ntp_tiles: fetch popular sites icons for 2nd NTP

components/ntp_tiles/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 "components/ntp_tiles/most_visited_sites.h"
 
 #include <algorithm>
 #include <set>
 #include <string>
 #include <utility>
@@ skipping 400 matching lines @@
     return;
 
   observer_->OnMostVisitedURLsAvailable(current_tiles_);
 }
 
 void MostVisitedSites::OnPopularSitesDownloaded(bool success) {
   if (!success) {
     LOG(WARNING) << "Download of popular sites failed";
     return;
   }
+
+  for (const PopularSites::Site& popular_site : popular_sites_->sites()) {
+    // Ignore callback; these icons will be seen on the *next* NTP.
+    icon_cacher_->StartFetch(popular_site,

[Marc Treib, 2017/02/27 12:33:35]

Hrm. So if the NTP gets closed in the meantime, then all this will get aborted,
right?

That indeed sounds like we should make MostVisitedSites long-lived. On desktop,
it already is, through being owned by InstantService.

[sfiera, 2017/02/27 12:51:33]

Yes. But that's already true of PopularSites as a whole.

[fhorschig, 2017/02/27 15:40:28]

+1 to making MVS/PS long-lived.
(Maybe not in this CL, though)

+                             base::Bind([](bool newly_available) {}));

[Marc Treib, 2017/02/27 12:33:35]

Can we just pass a null Callback here?

[sfiera, 2017/02/27 12:51:33]

Do you mean to change the signature of StartFetch(), or some other way to make
this callback?

[Marc Treib, 2017/02/27 13:09:35]

No, I mean just passing "PopularSites::FinishedCallback()" which IMO would be a
bit easier to read than the "empty" Bind() call.
(But then PopularSites would have to check the callback for validity before
calling it, so maybe that's more churn than it's worth.)

[fhorschig, 2017/02/27 15:40:28]

Agree to the readability but I think it's more likely that 
we drop the callback in the future than checking it for validity.
We currently use it only for warning, testing and debugging.
(And all of those uses are debatable...)

+  }
 }
 
 void MostVisitedSites::OnIconMadeAvailable(const GURL& site_url,
                                            bool newly_available) {
   if (newly_available)
     observer_->OnIconMadeAvailable(site_url);
 }
 
 void MostVisitedSites::TopSitesLoaded(TopSites* top_sites) {}
 
 void MostVisitedSites::TopSitesChanged(TopSites* top_sites,
                                        ChangeReason change_reason) {
   if (mv_source_ == NTPTileSource::TOP_SITES) {
     // The displayed tiles are invalidated.
     InitiateTopSitesQuery();
   }
 }
 
 }  // namespace ntp_tiles