[NTP] Use server thumbnails from SuggestionsService on Android NTP.

chrome/browser/android/most_visited_sites.cc

Index: chrome/browser/android/most_visited_sites.cc
diff --git a/chrome/browser/android/most_visited_sites.cc b/chrome/browser/android/most_visited_sites.cc
index 6fc326f7d9426cbad6c43d979ed6810d674a4b71..a44ee116d906c3a35992c69a83fc49e2f12b9713 100644
--- a/chrome/browser/android/most_visited_sites.cc
+++ b/chrome/browser/android/most_visited_sites.cc
@@ -98,9 +98,38 @@ void AddForcedURLOnUIThread(scoped_refptr<history::TopSites> top_sites,
   top_sites->AddForcedURL(url, base::Time::Now());
 }
 
+void OnSuggestionsThumbnailAvailable(
+    ScopedJavaGlobalRef<jobject>* j_callback,
+    const GURL& url,
+    const SkBitmap* bitmap) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  JNIEnv* env = AttachCurrentThread();
+
+  ScopedJavaGlobalRef<jobject>* j_bitmap_ref =
+      new ScopedJavaGlobalRef<jobject>();
+  if (bitmap) {
+    j_bitmap_ref->Reset(
+        env,
+        gfx::ConvertToJavaBitmap(bitmap).obj());
+  }
+
+  Java_ThumbnailCallback_onMostVisitedURLsThumbnailAvailable(
+      env, j_callback->obj(), j_bitmap_ref->obj());
+}
+
+void GetSuggestionsThumbnailOnUIThread(
+  const GURL& gurl,

[Ted C, 2014/05/28 00:21:59]

+2 for the param lines

[Mathieu, 2014/05/28 02:48:17]

Done.

+  SuggestionsService* suggestions_service,
+  ScopedJavaGlobalRef<jobject>* j_callback) {
+  suggestions_service->GetPageThumbnail(gurl,
+      base::Bind(&OnSuggestionsThumbnailAvailable,
+                 base::Owned(new ScopedJavaGlobalRef<jobject>(*j_callback))));
+}
+
 void GetUrlThumbnailTask(
     std::string url_string,
     scoped_refptr<TopSites> top_sites,
+    Profile* profile,
     ScopedJavaGlobalRef<jobject>* j_callback) {
   JNIEnv* env = AttachCurrentThread();
 
@@ -123,6 +152,17 @@ void GetUrlThumbnailTask(
     BrowserThread::PostTask(
         BrowserThread::UI, FROM_HERE,
         base::Bind(AddForcedURLOnUIThread, top_sites, gurl));
+
+    SuggestionsService* service =
+        SuggestionsServiceFactory::GetForProfile(profile);

[Ted C, 2014/05/28 00:21:59]

BrowserContextKeyedBaseFactory is NonThreadSafe, so I don't think you can do
this here.

Maybe this is always being called on the UI thread, so this doesn't matter, but
then we are doing a bunch of UI thread work below unnecessarily and we should
assert we are on the UI thread to prevent this breaking in the future.

[Mathieu, 2014/05/28 02:48:17]

Ah, thanks. Passing the profile pointer instead should be good then? Have a
look.

[Ted C, 2014/05/28 17:45:38]

I don't think profile is thread safe either.  And GetForProfile shouldn't be
called on anything put the UI thread either (ironically it like
SuggestionsServiceFactory might have overriden just enough methods that the base
ThreadChecker calls are all being skipped an no DCHECKs are being hit).

top_sites is passed a ref pointer and is thread safe, but I don't think you can
make the same assumptions with profile.

I think you need to get back to the MostVisitedSites object on the UI thread to
do what you want instead of passing these values.

+    if (service) {
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(GetSuggestionsThumbnailOnUIThread, gurl, service,
+                     base::Owned(
+                         new ScopedJavaGlobalRef<jobject>(*j_callback))));
+      return;
+    }
   }
 
   // Since j_callback is owned by this callback, when the callback falls out of
@@ -191,7 +231,7 @@ void MostVisitedSites::GetURLThumbnail(JNIEnv* env,
       BrowserThread::DB, FROM_HERE, base::Bind(
           &GetUrlThumbnailTask,
           url_string,
-          top_sites, base::Owned(j_callback)));
+          top_sites, profile_, base::Owned(j_callback)));
 }
 
 void MostVisitedSites::BlacklistUrl(JNIEnv* env,