Ensure PopularSite icon availability in ntp_tiles.

Previously, MostVisitedSites dumped all PopularSites information into
its own observer, then let Java ensure availability of icons. Popular
Sites on iOS needs these icons cached too, so the component should
handle icon availability itself.

MostVisitedSites::Observer::OnPopularURLsAvailable() is removed and
MostVisitedSites::Observer::OnIconMadeAvailable() is added in its place.

Note that FaviconHelper.ensureIconIsAvailable() hasn't been removed in
this CL, as it's still used to load favicons for snippets. Of course,
one day, iOS will need to do that too, so an eventual plan to share
IconCacher and broaden its interface makes sense (maybe coupled with a
move to the favicon component?).

BUG=631990

Review-Url: https://codereview.chromium.org/2388783004
Cr-Commit-Position: refs/heads/master@{#427100}

chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java

@@ -1047,27 +1047,8 @@ public class NewTabPageView extends FrameLayout
     }
 
     @Override
-    public void onPopularURLsAvailable(
-            String[] urls, String[] faviconUrls, String[] largeIconUrls) {
-        for (int i = 0; i < urls.length; i++) {
-            final String url = urls[i];
-            boolean useLargeIcon = !largeIconUrls[i].isEmpty();
-            // Only fetch one of favicon or large icon based on what is required on the NTP.
-            // The other will be fetched on visiting the site.
-            String iconUrl = useLargeIcon ? largeIconUrls[i] : faviconUrls[i];
-            if (iconUrl.isEmpty()) continue;
-
-            IconAvailabilityCallback callback = new IconAvailabilityCallback() {
-                @Override
-                public void onIconAvailabilityChecked(boolean newlyAvailable) {
-                    if (newlyAvailable) {
-                        mMostVisitedDesign.onIconUpdated(url);
-                    }
-                }
-            };
-            mManager.ensureIconIsAvailable(
-                    url, iconUrl, useLargeIcon, /*isTemporary=*/false, callback);
-        }
+    public void onIconMadeAvailable(String siteUrl) {
+        mMostVisitedDesign.onIconUpdated(siteUrl);
     }
 
     /**

chrome/android/java/src/org/chromium/chrome/browser/profiles/MostVisitedSites.java

@@ -31,15 +31,13 @@ public class MostVisitedSites {
                 String[] whitelistIconPaths, int[] sources);
 
         /**
-         * This is called when the list of popular URLs is initially available or updated.
+         * This is called when a previously uncached icon has been fetched.
          * Parameters guaranteed to be non-null.
          *
-         * @param urls Array of popular URLs.
-         * @param faviconUrls Array of URLs for the corresponding favicons (if known).
+         * @param siteUrl URL of site with newly-cached icon.
          */
         @CalledByNative("MostVisitedURLsObserver")
-        public void onPopularURLsAvailable(
-                String[] urls, String[] faviconUrls, String[] largeIconUrls);
+        public void onIconMadeAvailable(String siteUrl);
     }
 
     /**
@@ -80,11 +78,10 @@ public class MostVisitedSites {
                 }
             }
             @Override
-            public void onPopularURLsAvailable(
-                    String[] urls, String[] faviconUrls, String[] largeIconUrls) {
+            public void onIconMadeAvailable(String siteUrl) {
                 // Don't notify observer if we've already been destroyed.
                 if (mNativeMostVisitedSitesBridge != 0) {
-                    observer.onPopularURLsAvailable(urls, faviconUrls, largeIconUrls);
+                    observer.onIconMadeAvailable(siteUrl);
                 }
             }
         };

chrome/browser/android/ntp/most_visited_sites_bridge.cc

@@ -15,9 +15,11 @@
 #include "base/memory/ptr_util.h"
 #include "chrome/browser/android/ntp/popular_sites.h"
 #include "chrome/browser/browser_process.h"
+#include "chrome/browser/favicon/favicon_service_factory.h"
 #include "chrome/browser/history/top_sites_factory.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/profiles/profile_android.h"
+#include "chrome/browser/search/suggestions/image_decoder_impl.h"
 #include "chrome/browser/search/suggestions/suggestions_service_factory.h"
 #include "chrome/browser/search_engines/template_url_service_factory.h"
 #include "chrome/browser/supervised_user/supervised_user_service.h"
@@ -25,6 +27,8 @@
 #include "chrome/browser/supervised_user/supervised_user_url_filter.h"
 #include "chrome/browser/thumbnails/thumbnail_list_source.h"
 #include "components/history/core/browser/top_sites.h"
+#include "components/image_fetcher/image_fetcher_impl.h"
+#include "components/ntp_tiles/icon_cacher.h"
 #include "components/ntp_tiles/metrics.h"
 #include "components/ntp_tiles/popular_sites.h"
 #include "components/safe_json/safe_json_parser.h"
@@ -36,6 +40,7 @@
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF8;
+using base::android::ConvertUTF8ToJavaString;
 using base::android::JavaParamRef;
 using base::android::ScopedJavaGlobalRef;
 using base::android::ScopedJavaLocalRef;
@@ -104,8 +109,7 @@ class MostVisitedSitesBridge::JavaObserver : public MostVisitedSites::Observer {
 
   void OnMostVisitedURLsAvailable(const NTPTilesVector& tiles) override;
 
-  void OnPopularURLsAvailable(
-      const MostVisitedSites::PopularSitesVector& sites) override;
+  void OnIconMadeAvailable(const GURL& site_url) override;
 
  private:
   ScopedJavaGlobalRef<jobject> observer_;
@@ -143,21 +147,11 @@ void MostVisitedSitesBridge::JavaObserver::OnMostVisitedURLsAvailable(
       ToJavaIntArray(env, sources));
 }
 
-void MostVisitedSitesBridge::JavaObserver::OnPopularURLsAvailable(
-    const MostVisitedSites::PopularSitesVector& sites) {
+void MostVisitedSitesBridge::JavaObserver::OnIconMadeAvailable(
+    const GURL& site_url) {
   JNIEnv* env = AttachCurrentThread();
-  std::vector<std::string> urls;
-  std::vector<std::string> favicon_urls;
-  std::vector<std::string> large_icon_urls;
-  for (const auto& site : sites) {
-    urls.emplace_back(site.url.spec());
-    favicon_urls.emplace_back(site.favicon_url.spec());
-    large_icon_urls.emplace_back(site.large_icon_url.spec());
-  }
-  Java_MostVisitedURLsObserver_onPopularURLsAvailable(
-      env, observer_, ToJavaArrayOfStrings(env, urls),
-      ToJavaArrayOfStrings(env, favicon_urls),
-      ToJavaArrayOfStrings(env, large_icon_urls));
+  Java_MostVisitedURLsObserver_onIconMadeAvailable(
+      env, observer_, ConvertUTF8ToJavaString(env, site_url.spec()));
 }
 
 MostVisitedSitesBridge::MostVisitedSitesBridge(Profile* profile)
@@ -173,10 +167,18 @@ MostVisitedSitesBridge::MostVisitedSitesBridge(Profile* profile)
                         profile->GetRequestContext(),
                         ChromePopularSites::GetDirectory(),
                         base::Bind(safe_json::SafeJsonParser::Parse)),
+                    base::MakeUnique<ntp_tiles::IconCacher>(
+                        FaviconServiceFactory::GetForProfile(
+                            profile,
+                            ServiceAccessType::IMPLICIT_ACCESS),
+                        base::MakeUnique<image_fetcher::ImageFetcherImpl>(
+                            base::MakeUnique<suggestions::ImageDecoderImpl>(),
+                            profile->GetRequestContext())),
                     &supervisor_) {
   // Register the thumbnails debugging page.
   // TODO(sfiera): find thumbnails a home. They don't belong here.
   content::URLDataSource::Add(profile, new ThumbnailListSource(profile));
+  DCHECK(!profile->IsOffTheRecord());
 }
 
 MostVisitedSitesBridge::~MostVisitedSitesBridge() {}

components/ntp_tiles/BUILD.gn

@@ -10,6 +10,8 @@ static_library("ntp_tiles") {
   sources = [
     "constants.cc",
     "constants.h",
+    "icon_cacher.cc",
+    "icon_cacher.h",
     "metrics.cc",
     "metrics.h",
     "most_visited_sites.cc",
@@ -31,7 +33,10 @@ static_library("ntp_tiles") {
   ]
   deps = [
     "//components/data_use_measurement/core",
+    "//components/favicon/core",
+    "//components/favicon_base",
     "//components/google/core/browser",
+    "//components/image_fetcher",
     "//components/pref_registry",
     "//components/prefs",
     "//components/search_engines",
@@ -47,12 +52,18 @@ static_library("ntp_tiles") {
 source_set("unit_tests") {
   testonly = true
   sources = [
+    "icon_cacher_unittest.cc",
     "most_visited_sites_unittest.cc",
   ]
 
   deps = [
     ":ntp_tiles",
+    "//components/favicon/core",
+    "//components/favicon_base",
+    "//components/image_fetcher",
+    "//testing/gmock",
     "//testing/gtest",
+    "//ui/gfx:test_support",
   ]
 }
 

components/ntp_tiles/DEPS

 include_rules = [
   "+components/data_use_measurement/core",
+  "+components/favicon",
+  "+components/favicon_base",
   "+components/google/core/browser",
+  "+components/image_fetcher",
   "+components/history/core/browser",
   "+components/pref_registry",
   "+components/prefs",

components/ntp_tiles/icon_cacher.cc

+// Copyright 2016 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/icon_cacher.h"
+
+#include "components/favicon/core/favicon_service.h"
+#include "components/favicon/core/favicon_util.h"
+#include "components/favicon_base/favicon_types.h"
+#include "components/favicon_base/favicon_util.h"
+#include "components/image_fetcher/image_fetcher.h"
+#include "ui/gfx/image/image.h"
+#include "url/gurl.h"
+
+namespace ntp_tiles {
+
+namespace {
+
+favicon_base::IconType IconType(const PopularSites::Site& site) {
+  return site.large_icon_url.is_valid() ? favicon_base::TOUCH_ICON
+                                        : favicon_base::FAVICON;
+}
+
+const GURL& IconURL(const PopularSites::Site& site) {
+  return site.large_icon_url.is_valid() ? site.large_icon_url
+                                        : site.favicon_url;
+}
+
+}  // namespace
+
+IconCacher::IconCacher(
+    favicon::FaviconService* favicon_service,
+    std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher)
+    : favicon_service_(favicon_service),
+      image_fetcher_(std::move(image_fetcher)) {
+  image_fetcher_->SetDataUseServiceName(
+      data_use_measurement::DataUseUserData::NTP_TILES);
+}
+
+IconCacher::~IconCacher() = default;
+
+void IconCacher::StartFetch(PopularSites::Site site,
+                            const base::Callback<void(bool)>& done) {
+  favicon::GetFaviconImageForPageURL(
+      favicon_service_, site.url, IconType(site),
+      base::Bind(&IconCacher::OnGetFaviconImageForPageURLFinished,
+                 base::Unretained(this), std::move(site), done),
+      &tracker_);
+}
+
+void IconCacher::OnGetFaviconImageForPageURLFinished(
+    PopularSites::Site site,
+    const base::Callback<void(bool)>& done,
+    const favicon_base::FaviconImageResult& result) {
+  if (!result.image.IsEmpty()) {
+    done.Run(false);
+    return;
+  }
+
+  image_fetcher_->StartOrQueueNetworkRequest(
+      std::string(), IconURL(site),
+      base::Bind(&IconCacher::OnFaviconDownloaded, base::Unretained(this), site,
+                 done));
+}
+
+void IconCacher::OnFaviconDownloaded(PopularSites::Site site,
+                                     const base::Callback<void(bool)>& done,
+                                     const std::string& id,
+                                     const gfx::Image& fetched_image) {
+  if (fetched_image.IsEmpty()) {
+    done.Run(false);
+    return;
+  }
+
+  gfx::Image image = fetched_image;
+  favicon_base::SetFaviconColorSpace(&image);
+  favicon_service_->SetFavicons(site.url, IconURL(site), IconType(site), image);
+  done.Run(true);
+}
+
+}  // namespace ntp_tiles

components/ntp_tiles/icon_cacher.h

+// Copyright 2016 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.
+
+#ifndef COMPONENTS_NTP_TILES_ICON_CACHER_H_
+#define COMPONENTS_NTP_TILES_ICON_CACHER_H_
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "base/task/cancelable_task_tracker.h"
+#include "components/ntp_tiles/popular_sites.h"
+
+namespace favicon {
+class FaviconService;
+}  // namespace favicon
+
+namespace favicon_base {
+struct FaviconImageResult;
+}  // namespace favicon_base
+
+namespace gfx {
+class Image;
+}  // namespace gfx
+
+namespace image_fetcher {
+class ImageFetcher;
+}  // namespace image_fetcher
+
+namespace ntp_tiles {
+
+// Ensures that a Popular Sites icon is cached, downloading and saving it if
+// not.
+//
+// Does not provide any way to get a fetched favicon; use the FaviconService for
+// that. All this class does is guarantee that FaviconService will be able to
+// get you an icon (if it exists).
+class IconCacher {
+ public:
+  IconCacher(favicon::FaviconService* favicon_service,
+             std::unique_ptr<image_fetcher::ImageFetcher> image_fetcher);
+  ~IconCacher();
+
+  // Fetches the icon if necessary, then invokes |done| with true if it was
+  // newly fetched (false if it was already cached or could not be fetched).
+  void StartFetch(PopularSites::Site site,
+                  const base::Callback<void(bool)>& done);
+
+ private:
+  void OnGetFaviconImageForPageURLFinished(
+      PopularSites::Site site,
+      const base::Callback<void(bool)>& done,
+      const favicon_base::FaviconImageResult& result);
+
+  void OnFaviconDownloaded(PopularSites::Site site,
+                           const base::Callback<void(bool)>& done,
+                           const std::string& id,
+                           const gfx::Image& fetched_image);
+
+  base::CancelableTaskTracker tracker_;
+  favicon::FaviconService* const favicon_service_;
+  std::unique_ptr<image_fetcher::ImageFetcher> const image_fetcher_;
+
+  DISALLOW_COPY_AND_ASSIGN(IconCacher);
+};
+
+}  // namespace ntp_tiles
+
+#endif  // COMPONENTS_NTP_TILES_ICON_CACHER_H_

components/ntp_tiles/icon_cacher_unittest.cc

+// Copyright 2016 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/icon_cacher.h"
+
+#include "base/files/scoped_temp_dir.h"
+#include "base/memory/ptr_util.h"
+#include "base/run_loop.h"
+#include "base/threading/thread_task_runner_handle.h"
+#include "components/favicon/core/favicon_client.h"
+#include "components/favicon/core/favicon_service.h"
+#include "components/favicon/core/favicon_util.h"
+#include "components/history/core/browser/history_database_params.h"
+#include "components/history/core/browser/history_service.h"
+#include "components/image_fetcher/image_fetcher.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "ui/gfx/image/image_unittest_util.h"
+
+using ::testing::_;
+using ::testing::InSequence;
+using ::testing::MockFunction;
+using ::testing::Return;
+
+namespace ntp_tiles {
+namespace {
+
+class MockImageFetcher : public image_fetcher::ImageFetcher {
+ public:
+  MOCK_METHOD1(SetImageFetcherDelegate,
+               void(image_fetcher::ImageFetcherDelegate* delegate));
+  MOCK_METHOD1(SetDataUseServiceName,
+               void(image_fetcher::ImageFetcher::DataUseServiceName name));
+  MOCK_METHOD3(StartOrQueueNetworkRequest,
+               void(const std::string& id,
+                    const GURL& image_url,
+                    base::Callback<void(const std::string& id,
+                                        const gfx::Image& image)> callback));
+};
+
+class IconCacherTest : public ::testing::Test {
+ protected:
+  IconCacherTest()
+      : site_(base::string16(),  // title, unused
+              GURL("http://url.google/"),
+              GURL("http://url.google/icon.png"),
+              GURL("http://url.google/favicon.ico"),
+              GURL()),  // thumbnail, unused
+        image_fetcher_(new ::testing::StrictMock<MockImageFetcher>),
+        favicon_service_(/*favicon_client=*/nullptr, &history_service_) {
+    CHECK(history_dir_.CreateUniqueTempDir());
+    CHECK(history_service_.Init(
+        history::HistoryDatabaseParams(history_dir_.GetPath(), 0, 0)));
+  }
+
+  void PreloadIcon(const GURL& url,
+                   const GURL& icon_url,
+                   favicon_base::IconType icon_type,
+                   int width,
+                   int height) {
+    favicon_service_.SetFavicons(url, icon_url, icon_type,
+                                 gfx::test::CreateImage(width, height));
+  }
+
+  bool IconIsCachedFor(const GURL& url, favicon_base::IconType icon_type) {
+    base::CancelableTaskTracker tracker;
+    bool is_cached;
+    base::RunLoop loop;
+    favicon::GetFaviconImageForPageURL(
+        &favicon_service_, url, icon_type,
+        base::Bind(
+            [](bool* is_cached, base::RunLoop* loop,
+               const favicon_base::FaviconImageResult& result) {
+              *is_cached = !result.image.IsEmpty();
+              loop->Quit();
+            },
+            &is_cached, &loop),
+        &tracker);
+    loop.Run();
+    return is_cached;
+  }
+
+  base::MessageLoop message_loop_;
+  PopularSites::Site site_;
+  std::unique_ptr<MockImageFetcher> image_fetcher_;
+  base::ScopedTempDir history_dir_;
+  history::HistoryService history_service_;
+  favicon::FaviconService favicon_service_;
+};
+
+template <typename Fn>
+base::Callback<Fn> BindMockFunction(MockFunction<Fn>* function) {
+  return base::Bind(&MockFunction<Fn>::Call, base::Unretained(function));
+}
+
+ACTION(FailFetch) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(arg2, arg0, gfx::Image()));
+}
+
+ACTION_P2(PassFetch, width, height) {
+  base::ThreadTaskRunnerHandle::Get()->PostTask(
+      FROM_HERE, base::Bind(arg2, arg0, gfx::test::CreateImage(width, height)));
+}
+
+ACTION_P(Quit, run_loop) {
+  run_loop->Quit();
+}
+
+TEST_F(IconCacherTest, LargeCached) {
+  MockFunction<void(bool)> done;
+  base::RunLoop loop;
+  {
+    InSequence s;
+    EXPECT_CALL(*image_fetcher_,
+                SetDataUseServiceName(
+                    data_use_measurement::DataUseUserData::NTP_TILES));
+    EXPECT_CALL(done, Call(false)).WillOnce(Quit(&loop));
+  }
+  PreloadIcon(site_.url, site_.large_icon_url, favicon_base::TOUCH_ICON, 128,
+              128);
+
+  IconCacher cacher(&favicon_service_, std::move(image_fetcher_));
+  cacher.StartFetch(site_, BindMockFunction(&done));
+  loop.Run();
+  EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
+  EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
+}
+
+TEST_F(IconCacherTest, LargeNotCachedAndFetchSucceeded) {
+  MockFunction<void(bool)> done;
+  base::RunLoop loop;
+  {
+    InSequence s;
+    EXPECT_CALL(*image_fetcher_,
+                SetDataUseServiceName(
+                    data_use_measurement::DataUseUserData::NTP_TILES));
+    EXPECT_CALL(*image_fetcher_,
+                StartOrQueueNetworkRequest(_, site_.large_icon_url, _))
+        .WillOnce(PassFetch(128, 128));
+    EXPECT_CALL(done, Call(true)).WillOnce(Quit(&loop));
+  }
+
+  IconCacher cacher(&favicon_service_, std::move(image_fetcher_));
+  cacher.StartFetch(site_, BindMockFunction(&done));
+  loop.Run();
+  EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
+  EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
+}
+
+TEST_F(IconCacherTest, SmallNotCachedAndFetchSucceeded) {
+  site_.large_icon_url = GURL();
+
+  MockFunction<void(bool)> done;
+  base::RunLoop loop;
+  {
+    InSequence s;
+    EXPECT_CALL(*image_fetcher_,
+                SetDataUseServiceName(
+                    data_use_measurement::DataUseUserData::NTP_TILES));
+    EXPECT_CALL(*image_fetcher_,
+                StartOrQueueNetworkRequest(_, site_.favicon_url, _))
+        .WillOnce(PassFetch(128, 128));
+    EXPECT_CALL(done, Call(true)).WillOnce(Quit(&loop));
+  }
+
+  IconCacher cacher(&favicon_service_, std::move(image_fetcher_));
+  cacher.StartFetch(site_, BindMockFunction(&done));
+  loop.Run();
+  EXPECT_TRUE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
+  EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
+}
+
+TEST_F(IconCacherTest, LargeNotCachedAndFetchFailed) {
+  MockFunction<void(bool)> done;
+  base::RunLoop loop;
+  {
+    InSequence s;
+    EXPECT_CALL(*image_fetcher_,
+                SetDataUseServiceName(
+                    data_use_measurement::DataUseUserData::NTP_TILES));
+    EXPECT_CALL(*image_fetcher_,
+                StartOrQueueNetworkRequest(_, site_.large_icon_url, _))
+        .WillOnce(FailFetch());
+    EXPECT_CALL(done, Call(false)).WillOnce(Quit(&loop));
+  }
+
+  IconCacher cacher(&favicon_service_, std::move(image_fetcher_));
+  cacher.StartFetch(site_, BindMockFunction(&done));
+  loop.Run();
+  EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::FAVICON));
+  EXPECT_FALSE(IconIsCachedFor(site_.url, favicon_base::TOUCH_ICON));
+}
+
+}  // namespace
+}  // namespace ntp_tiles

components/ntp_tiles/most_visited_sites.cc

@@ -21,6 +21,7 @@
 #include "base/strings/utf_string_conversions.h"
 #include "components/history/core/browser/top_sites.h"
 #include "components/ntp_tiles/constants.h"
+#include "components/ntp_tiles/icon_cacher.h"
 #include "components/ntp_tiles/metrics.h"
 #include "components/ntp_tiles/pref_names.h"
 #include "components/ntp_tiles/switches.h"
@@ -84,11 +85,13 @@ MostVisitedSites::MostVisitedSites(PrefService* prefs,
                                    scoped_refptr<history::TopSites> top_sites,
                                    SuggestionsService* suggestions,
                                    std::unique_ptr<PopularSites> popular_sites,
+                                   std::unique_ptr<IconCacher> icon_cacher,
                                    MostVisitedSitesSupervisor* supervisor)
     : prefs_(prefs),
       top_sites_(top_sites),
       suggestions_service_(suggestions),
       popular_sites_(std::move(popular_sites)),
+      icon_cacher_(std::move(icon_cacher)),
       supervisor_(supervisor),
       observer_(nullptr),
       num_sites_(0),
@@ -345,6 +348,9 @@ NTPTilesVector MostVisitedSites::CreatePopularSitesTiles(
       tile.source = NTPTileSource::POPULAR;
 
       popular_sites_tiles.push_back(std::move(tile));
+      icon_cacher_->StartFetch(
+          popular_site, base::Bind(&MostVisitedSites::OnIconMadeAvailable,
+                                   base::Unretained(this), popular_site.url));
       if (popular_sites_tiles.size() >= num_popular_sites_tiles)
         break;
     }
@@ -410,14 +416,16 @@ void MostVisitedSites::OnPopularSitesAvailable(bool success) {
     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());
-
   // Re-build the tile list. Once done, this will notify the observer.
   BuildCurrentTiles();
 }
 
+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,

components/ntp_tiles/most_visited_sites.h

@@ -34,6 +34,8 @@ class PrefRegistrySyncable;
 
 namespace ntp_tiles {
 
+class IconCacher;
+
 // Shim interface for SupervisedUserService.
 class MostVisitedSitesSupervisor {
  public:
@@ -86,7 +88,8 @@ class MostVisitedSites : public history::TopSitesObserver,
   class Observer {
    public:
     virtual void OnMostVisitedURLsAvailable(const NTPTilesVector& tiles) = 0;
-    virtual void OnPopularURLsAvailable(const PopularSitesVector& sites) {}
+    // TODO(sfiera): make this method required after iOS implements it:
+    virtual void OnIconMadeAvailable(const GURL& site_url) {}
 
    protected:
     virtual ~Observer() {}
@@ -101,6 +104,7 @@ class MostVisitedSites : public history::TopSitesObserver,
                    scoped_refptr<history::TopSites> top_sites,
                    suggestions::SuggestionsService* suggestions,
                    std::unique_ptr<PopularSites> popular_sites,
+                   std::unique_ptr<IconCacher> icon_cacher,
                    MostVisitedSitesSupervisor* supervisor);
 
   ~MostVisitedSites() override;
@@ -162,6 +166,8 @@ class MostVisitedSites : public history::TopSitesObserver,
 
   void OnPopularSitesAvailable(bool success);
 
+  void OnIconMadeAvailable(const GURL& site_url, bool newly_available);
+
   // history::TopSitesObserver implementation.
   void TopSitesLoaded(history::TopSites* top_sites) override;
   void TopSitesChanged(history::TopSites* top_sites,
@@ -171,6 +177,7 @@ class MostVisitedSites : public history::TopSitesObserver,
   scoped_refptr<history::TopSites> top_sites_;
   suggestions::SuggestionsService* suggestions_service_;
   std::unique_ptr<PopularSites> const popular_sites_;
+  std::unique_ptr<IconCacher> const icon_cacher_;
   MostVisitedSitesSupervisor* supervisor_;
 
   Observer* observer_;

components/ntp_tiles/popular_sites.cc

@@ -279,7 +279,6 @@ void PopularSites::OnReadFileDone(std::unique_ptr<std::string> data,
 
 void PopularSites::FetchPopularSites() {
   fetcher_ = URLFetcher::Create(pending_url_, URLFetcher::GET, this);
-  // TODO(sfiera): Count the downloaded bytes of icons fetched by popular sites.
   data_use_measurement::DataUseUserData::AttachToFetcher(
       fetcher_.get(), data_use_measurement::DataUseUserData::NTP_TILES);
   fetcher_->SetRequestContext(download_context_);

ios/chrome/browser/ntp_tiles/ios_most_visited_sites_factory.cc

@@ -6,18 +6,36 @@
 
 #include "base/memory/ptr_util.h"
 #include "components/history/core/browser/top_sites.h"
+#include "components/image_fetcher/image_fetcher_impl.h"
+#include "components/keyed_service/core/service_access_type.h"
+#include "components/ntp_tiles/icon_cacher.h"
 #include "components/ntp_tiles/most_visited_sites.h"
 #include "ios/chrome/browser/browser_state/chrome_browser_state.h"
+#include "ios/chrome/browser/favicon/favicon_service_factory.h"
 #include "ios/chrome/browser/history/top_sites_factory.h"
 #include "ios/chrome/browser/ntp_tiles/ios_popular_sites_factory.h"
+#include "ios/chrome/browser/suggestions/ios_image_decoder_impl.h"
 #include "ios/chrome/browser/suggestions/suggestions_service_factory.h"
+#include "ios/web/public/web_thread.h"
 
 std::unique_ptr<ntp_tiles::MostVisitedSites>
 IOSMostVisitedSitesFactory::NewForBrowserState(
     ios::ChromeBrowserState* browser_state) {
+  scoped_refptr<base::SequencedTaskRunner> task_runner =
+      web::WebThread::GetBlockingPool()
+          ->GetSequencedTaskRunnerWithShutdownBehavior(
+              base::SequencedWorkerPool::GetSequenceToken(),
+              base::SequencedWorkerPool::CONTINUE_ON_SHUTDOWN);
   return base::MakeUnique<ntp_tiles::MostVisitedSites>(
       browser_state->GetPrefs(),
       ios::TopSitesFactory::GetForBrowserState(browser_state),
       suggestions::SuggestionsServiceFactory::GetForBrowserState(browser_state),
-      IOSPopularSitesFactory::NewForBrowserState(browser_state), nil);
+      IOSPopularSitesFactory::NewForBrowserState(browser_state),
+      base::MakeUnique<ntp_tiles::IconCacher>(
+          ios::FaviconServiceFactory::GetForBrowserState(
+              browser_state, ServiceAccessType::IMPLICIT_ACCESS),
+          base::MakeUnique<image_fetcher::ImageFetcherImpl>(
+              suggestions::CreateIOSImageDecoder(task_runner),
+              browser_state->GetRequestContext())),
+      nil);
 }