Ensure PopularSite icon availability in ntp_tiles.

chrome/browser/android/ntp/most_visited_sites_bridge.cc

Index: chrome/browser/android/ntp/most_visited_sites_bridge.cc
diff --git a/chrome/browser/android/ntp/most_visited_sites_bridge.cc b/chrome/browser/android/ntp/most_visited_sites_bridge.cc
index aef8e0730b5a20b78f7bbb674c3786cf04125988..233478daa2c204a1728dfd9a181949bd8c3fd9ae 100644
--- a/chrome/browser/android/ntp/most_visited_sites_bridge.cc
+++ b/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/popular_sites.h"
 #include "components/safe_json/safe_json_parser.h"
 #include "content/public/browser/browser_thread.h"
@@ -35,6 +39,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;
@@ -102,8 +107,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_;
@@ -141,21 +145,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)
@@ -171,6 +165,13 @@ 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),

[Marc Treib, 2016/10/12 09:11:27]

This will return nullptr in incognito, and IconCacher doesn't seem to handle
that. We prooobably can't get here in incognito? But I'd prefer that to be more
explicit, i.e. a DCHECK or something.

[sfiera, 2016/10/13 09:06:45]

Is that really the case? You can still see already-cached favicons in incognito
mode, so is EXPLICIT_ACCESS the right setting? (Perhaps we still shouldn't be
issuing any requests on the user's behalf in incognito mode)

[Marc Treib, 2016/10/13 12:31:15]

See
https://cs.chromium.org/chromium/src/chrome/browser/favicon/favicon_service_f...
- it'll return null for IMPLICIT_ACCESS in incognito.
We *definitely* shouldn't store any favicons in incognito. But anyway, we
shouldn't ever get here in incognito. So I'd just add a DCHECK.

[sfiera, 2016/10/14 06:27:32]

…after NOTREACHED(). So it's not valid to get a FaviconService for
IMPLICIT_ACCESS in incognito.
I've added one, because yes, we don't get here in incognito, but I still feel a
little iffy about that. As far as I know, there isn't a requirement that we not
use MostVisitedSites in incognito mode, it just happens that we don't at
present.

[Marc Treib, 2016/10/14 08:46:21]

d'oh, obviously I'm blind.
Well, it seems that if we ever wanted to use MVS in incognito, that would
require more work (at least, we'd have to check that we don't do anything we
shouldn't). So IMO a DCHECK here is the right thing for now.

+                        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.