Ensure PopularSite icon availability in ntp_tiles.

chrome/browser/android/ntp/most_visited_sites_bridge.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_bridge.h"
 
 #include <utility>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
 #include "base/android/scoped_java_ref.h"
 #include "base/bind.h"
 #include "base/callback.h"
 #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"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
 #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"
 #include "content/public/browser/url_data_source.h"
 #include "jni/MostVisitedSites_jni.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "url/gurl.h"
 
 using base::android::AttachCurrentThread;
 using base::android::ConvertJavaStringToUTF8;
+using base::android::ConvertUTF8ToJavaString;
 using base::android::JavaParamRef;
 using base::android::ScopedJavaGlobalRef;
 using base::android::ScopedJavaLocalRef;
 using base::android::ToJavaArrayOfStrings;
 using base::android::ToJavaIntArray;
 using content::BrowserThread;
 using ntp_tiles::MostVisitedSites;
 using ntp_tiles::MostVisitedSitesSupervisor;
 using ntp_tiles::NTPTileSource;
 using suggestions::SuggestionsServiceFactory;
@@ skipping 47 matching lines @@
   if (supervisor_observer_)
     supervisor_observer_->OnBlockedSitesChanged();
 }
 
 class MostVisitedSitesBridge::JavaObserver : public MostVisitedSites::Observer {
  public:
   JavaObserver(JNIEnv* env, const JavaParamRef<jobject>& obj);
 
   void OnMostVisitedURLsAvailable(const NTPTilesVector& tiles) override;
 
-  void OnPopularURLsAvailable(
-      const MostVisitedSites::PopularSitesVector& sites) override;
+  void OnIconMadeAvailable(const GURL& site_url) override;
 
  private:
   ScopedJavaGlobalRef<jobject> observer_;
 
   DISALLOW_COPY_AND_ASSIGN(JavaObserver);
 };
 
 MostVisitedSitesBridge::JavaObserver::JavaObserver(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj)
@@ skipping 17 matching lines @@
     whitelist_icon_paths.emplace_back(tile.whitelist_icon_path.value());
     sources.emplace_back(static_cast<int>(tile.source));
   }
   Java_MostVisitedURLsObserver_onMostVisitedURLsAvailable(
       env, observer_, ToJavaArrayOfStrings(env, titles),
       ToJavaArrayOfStrings(env, urls),
       ToJavaArrayOfStrings(env, whitelist_icon_paths),
       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)
     : supervisor_(profile),
       most_visited_(profile->GetPrefs(),
                     TopSitesFactory::GetForProfile(profile),
                     SuggestionsServiceFactory::GetForProfile(profile),
                     base::MakeUnique<ntp_tiles::PopularSites>(
                         BrowserThread::GetBlockingPool(),
                         profile->GetPrefs(),
                         TemplateURLServiceFactory::GetForProfile(profile),
                         g_browser_process->variations_service(),
                         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.
   content::URLDataSource::Add(profile, new ThumbnailListSource(profile));
 }
 
 MostVisitedSitesBridge::~MostVisitedSitesBridge() {}
 
 void MostVisitedSitesBridge::Destroy(
     JNIEnv* env, const JavaParamRef<jobject>& obj) {
@@ skipping 58 matching lines @@
 }
 
 static jlong Init(JNIEnv* env,
                   const JavaParamRef<jobject>& obj,
                   const JavaParamRef<jobject>& jprofile) {
   MostVisitedSitesBridge* most_visited_sites =
       new MostVisitedSitesBridge(
           ProfileAndroid::FromProfileAndroid(jprofile));
   return reinterpret_cast<intptr_t>(most_visited_sites);
 }