Popular sites on the NTP: Favicons!

chrome/browser/android/favicon_helper.cc

Index: chrome/browser/android/favicon_helper.cc
diff --git a/chrome/browser/android/favicon_helper.cc b/chrome/browser/android/favicon_helper.cc
index 47512d5e82d08af81e93f58b27d90c2c00cd6836..a46f10bf18be332ff7075ce237e3e6aa651041b6 100644
--- a/chrome/browser/android/favicon_helper.cc
+++ b/chrome/browser/android/favicon_helper.cc
@@ -6,6 +6,8 @@
 
 #include <jni.h>
 
+#include <vector>
+
 #include "base/android/jni_android.h"
 #include "base/android/jni_array.h"
 #include "base/android/jni_string.h"
@@ -18,15 +20,19 @@
 #include "chrome/browser/profiles/profile_android.h"
 #include "chrome/browser/sync/profile_sync_service.h"
 #include "chrome/browser/sync/profile_sync_service_factory.h"
-#include "chrome/browser/sync/profile_sync_service_factory.h"
 #include "components/favicon/core/favicon_service.h"
+#include "components/favicon_base/favicon_util.h"
+#include "components/favicon_base/select_favicon_frames.h"
 #include "components/sync_driver/open_tabs_ui_delegate.h"
+#include "content/public/browser/web_contents.h"
 #include "jni/FaviconHelper_jni.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/color_analysis.h"
 #include "ui/gfx/color_utils.h"
+#include "ui/gfx/favicon_size.h"
+#include "ui/gfx/image/image_skia.h"
 
 using base::android::ScopedJavaGlobalRef;
 using base::android::ScopedJavaLocalRef;
@@ -88,6 +94,52 @@ void OnFaviconRawBitmapResultAvailable(
                                                j_icon_url.obj());
 }
 
+void OnFaviconDownloaded(ScopedJavaGlobalRef<jobject>* j_availability_callback,
+                         Profile* profile,
+                         const GURL& page_url,
+                         int download_request_id,
+                         int http_status_code,
+                         const GURL& image_url,
+                         const std::vector<SkBitmap>& bitmaps,
+                         const std::vector<gfx::Size>& original_sizes) {
+  bool success = !bitmaps.empty();
+  if (success) {
+    gfx::Image image = gfx::Image(CreateFaviconImageSkia(bitmaps,
+                                                         original_sizes,
+                                                         gfx::kFaviconSize,
+                                                         nullptr));
+    favicon_base::SetFaviconColorSpace(&image);
+    favicon::FaviconService* service = FaviconServiceFactory::GetForProfile(
+        profile, ServiceAccessType::IMPLICIT_ACCESS);
+    service->SetFavicons(page_url, image_url, favicon_base::FAVICON, image);
+  }
+
+  JNIEnv* env = AttachCurrentThread();
+  Java_FaviconAvailabilityCallback_onFaviconAvailabilityChecked(
+      env, j_availability_callback->obj(), success);
+}
+
+void OnFaviconImageResultAvailable(
+    ScopedJavaGlobalRef<jobject>* j_availability_callback,
+    Profile* profile,
+    content::WebContents* web_contents,
+    const GURL& page_url,
+    const GURL& favicon_url,
+    const favicon_base::FaviconImageResult& result) {
+  // If there already is a favicon, return immediately.
+  if (!result.image.IsEmpty()) {
+    JNIEnv* env = AttachCurrentThread();
+    Java_FaviconAvailabilityCallback_onFaviconAvailabilityChecked(
+        env, j_availability_callback->obj(), false);
+    return;
+  }
+
+  web_contents->DownloadImage(favicon_url, true, 0, false,
+                              base::Bind(OnFaviconDownloaded,
+                                         j_availability_callback,
+                                         profile, page_url));
+}
+
 }  // namespace
 
 static jlong Init(JNIEnv* env, jclass clazz) {
@@ -210,6 +262,37 @@ ScopedJavaLocalRef<jobject> FaviconHelper::GetSyncedFaviconImageForURL(
   return gfx::ConvertToJavaBitmap(&favicon_bitmap);
 }
 
+void FaviconHelper::EnsureFaviconIsAvailable(
+    JNIEnv* env,
+    jobject obj,
+    jobject j_profile,
+    jobject j_web_contents,
+    jstring j_page_url,
+    jstring j_favicon_url,
+    jobject j_availability_callback) {
+  Profile* profile = ProfileAndroid::FromProfileAndroid(j_profile);
+  DCHECK(profile);
+  content::WebContents* web_contents =
+      content::WebContents::FromJavaWebContents(j_web_contents);
+  DCHECK(web_contents);
+  GURL page_url = GURL(ConvertJavaStringToUTF8(env, j_page_url));

[Bernhard Bauer, 2015/08/11 09:37:25]

You can directly initialize the variable (as opposed to assigning it an
explicitly constructed GURL).

[Marc Treib, 2015/08/11 10:23:33]

I'm pretty sure that amounts to the exact same thing (i.e. no assignment will be
performed anyway). But still, nicer to just initialize directly. Done.

+  GURL favicon_url = GURL(ConvertJavaStringToUTF8(env, j_favicon_url));
+
+  ScopedJavaGlobalRef<jobject>* j_scoped_callback =

[Bernhard Bauer, 2015/08/11 09:37:25]

Where is this object destroyed?

[Marc Treib, 2015/08/11 10:23:33]

It wasn't :)
Now it's done in the Bind below via base::Owned.

[Bernhard Bauer, 2015/08/11 10:45:06]

Urr... except now it will be destroyed at the end of
OnFaviconImageResultAvailable(), which means you'll pass an invalid pointer to
the callback bound to OnFaviconDownloaded().

Really, you want something like base::Passed() here so there is no chance of a
leak or use-after-free, or just directly copy the ScopedJavaGlobalRef around
(it's just a wrapper around jobject, so copying isn't that expensive, and you
can use const-ref like we usually do).

[Marc Treib, 2015/08/11 11:22:41]

Durr. That was silly, sorry. (And good catch!)
Now passing around copies.

+      new ScopedJavaGlobalRef<jobject>();
+  j_scoped_callback->Reset(env, j_availability_callback);
+
+  // TODO(treib): Optimize this by creating a FaviconService::HasFavicon method
+  // so that we don't have to actually get the image.
+  favicon_base::FaviconImageCallback callback_runner =
+      base::Bind(&OnFaviconImageResultAvailable, j_scoped_callback,
+                 profile, web_contents, page_url, favicon_url);
+  favicon::FaviconService* service = FaviconServiceFactory::GetForProfile(
+      profile, ServiceAccessType::IMPLICIT_ACCESS);
+  service->GetFaviconImage(favicon_url, callback_runner,
+                           cancelable_task_tracker_.get());
+}
+
 FaviconHelper::~FaviconHelper() {}
 
 static jint GetDominantColorForBitmap(JNIEnv* env,
@@ -218,9 +301,9 @@ static jint GetDominantColorForBitmap(JNIEnv* env,
   if (!bitmap)
     return 0;
 
-    gfx::JavaBitmap bitmap_lock(bitmap);
-    SkBitmap skbitmap = gfx::CreateSkBitmapFromJavaBitmap(bitmap_lock);
-    return color_utils::CalculateKMeanColorOfBitmap(skbitmap);
+  gfx::JavaBitmap bitmap_lock(bitmap);
+  SkBitmap skbitmap = gfx::CreateSkBitmapFromJavaBitmap(bitmap_lock);
+  return color_utils::CalculateKMeanColorOfBitmap(skbitmap);
 }
 
 // static