Use Manifest.icons instead of favicon in ShortcutHelper when possible.

chrome/browser/android/shortcut_helper.cc

Index: chrome/browser/android/shortcut_helper.cc
diff --git a/chrome/browser/android/shortcut_helper.cc b/chrome/browser/android/shortcut_helper.cc
index a485d40ec62cb6b12304e5e7130fc5b5da01f1d8..e529477748f985b222936c9e7195dfc6758de114 100644
--- a/chrome/browser/android/shortcut_helper.cc
+++ b/chrome/browser/android/shortcut_helper.cc
@@ -5,12 +5,14 @@
 #include "chrome/browser/android/shortcut_helper.h"
 
 #include <jni.h>
+#include <limits>
 
 #include "base/android/jni_android.h"
 #include "base/android/jni_string.h"
 #include "base/basictypes.h"
 #include "base/location.h"
 #include "base/strings/string16.h"
+#include "base/strings/utf_string_conversions.h"
 #include "base/task/cancelable_task_tracker.h"
 #include "base/threading/worker_pool.h"
 #include "chrome/browser/android/tab_android.h"
@@ -25,12 +27,20 @@
 #include "content/public/common/frame_navigate_params.h"
 #include "content/public/common/manifest.h"
 #include "jni/ShortcutHelper_jni.h"
+#include "net/base/mime_util.h"
 #include "ui/gfx/android/java_bitmap.h"
 #include "ui/gfx/codec/png_codec.h"
 #include "ui/gfx/color_analysis.h"
 #include "ui/gfx/favicon_size.h"
+#include "ui/gfx/screen.h"
 #include "url/gurl.h"
 
+using content::Manifest;
+
+// Android's preferred icon size in DP is 48, as defined in
+// http://developer.android.com/design/style/iconography.html
+const int ShortcutHelper::kPreferredIconSizeInDp = 48;
+
 jlong Initialize(JNIEnv* env, jobject obj, jlong tab_android_ptr) {
   TabAndroid* tab = reinterpret_cast<TabAndroid*>(tab_android_ptr);
 
@@ -49,6 +59,11 @@ ShortcutHelper::ShortcutHelper(JNIEnv* env,
       url_(web_contents->GetURL()),
       display_(content::Manifest::DISPLAY_MODE_BROWSER),
       orientation_(blink::WebScreenOrientationLockDefault),
+      add_shortcut_requested_(false),
+      manifest_icon_status_(MANIFEST_ICON_STATUS_NONE),
+      preferred_icon_size_in_px_(kPreferredIconSizeInDp *

[gone, 2014/09/24 18:17:24]

does this calculation _have_ to go here?  makes the constructor look extremely
messy when you daisy chain functions in the initializer list

[mlamouri (slow - plz ping), 2014/09/24 18:36:49]

I possibly moved it there because of the value being |const|.

+          gfx::Screen::GetScreenFor(web_contents->GetNativeView())->
+              GetPrimaryDisplay().device_scale_factor()),
       weak_ptr_factory_(this) {
 }
 
@@ -97,6 +112,98 @@ void ShortcutHelper::OnDidGetWebApplicationInfo(
                                          weak_ptr_factory_.GetWeakPtr()));
 }
 
+bool ShortcutHelper::IconSizesContainsPreferredSize(
+    const std::vector<gfx::Size>& sizes) const {
+  for (size_t i = 0; i < sizes.size(); ++i) {
+    if (sizes[i].height() != sizes[i].width())
+      continue;
+    if (sizes[i].width() == preferred_icon_size_in_px_)
+      return true;
+  }
+
+  return false;
+}
+
+GURL ShortcutHelper::FindBestMatchingIcon(
+    const std::vector<Manifest::Icon>& icons, float density) const {
+  GURL url;
+  int delta = std::numeric_limits<int>::min();
+
+  for (size_t i = 0; i < icons.size(); ++i) {
+    if (icons[i].density != density)
+      continue;
+
+    const std::vector<gfx::Size>& sizes = icons[i].sizes;
+    for (size_t j = 0; j < sizes.size(); ++j) {
+      if (sizes[j].height() != sizes[j].width())
+        continue;
+      int d = sizes[j].width() - preferred_icon_size_in_px_;
+      if (d == 0)
+        return icons[i].src;
+      if (delta > 0 && d < 0)
+        continue;
+      if ((delta > 0 && d < delta) || (delta < 0 && d > delta)) {
+        url = icons[i].src;
+        delta = d;
+      }
+    }
+  }
+
+  return url;
+}
+
+// static
+std::vector<Manifest::Icon> ShortcutHelper::FilterIconsByType(
+    const std::vector<Manifest::Icon>& icons) {
+  std::vector<Manifest::Icon> result;
+
+  for (size_t i = 0; i < icons.size(); ++i) {
+    if (icons[i].type.is_null() ||

[gone, 2014/09/24 18:17:24]

any reason why you keep the icon if the type is null?

[mlamouri (slow - plz ping), 2014/09/24 18:36:49]

If the type is not specified but sizes are, I would prefer to consider the entry
as a valid candidate. Note that we will only use it if it happens that it is the
best size we found. If unfortunately the entry is of an invalid type, the
decoding will fail and we will fallback to the favicons.

The algorithm is not set in stone and is meant to evolve.

+        net::IsSupportedImageMimeType(
+            base::UTF16ToUTF8(icons[i].type.string()))) {
+      result.push_back(icons[i]);
+    }
+  }
+
+  return result;
+}
+
+GURL ShortcutHelper::FindBestMatchingIcon(
+    const std::vector<Manifest::Icon>& unfiltered_icons) const {
+  const float device_scale_factor =
+      gfx::Screen::GetScreenFor(web_contents()->GetNativeView())->
+          GetPrimaryDisplay().device_scale_factor();
+
+  GURL url;
+  std::vector<Manifest::Icon> icons = FilterIconsByType(unfiltered_icons);
+
+  // The first pass is to find the ideal icon. That icon is of the right size
+  // with the default density or the device's density.
+  for (size_t i = 0; i < icons.size(); ++i) {
+    if (icons[i].density == device_scale_factor &&
+        IconSizesContainsPreferredSize(icons[i].sizes)) {
+      return icons[i].src;
+    }
+
+    // If there is an icon with the right size but not the right density, keep
+    // it on the side and only use it if nothing better is found.
+    if (icons[i].density == Manifest::Icon::kDefaultDensity &&
+        IconSizesContainsPreferredSize(icons[i].sizes)) {
+      url = icons[i].src;
+    }
+  }
+
+  // Second pass will try to find the best suitable icon for the device's scale
+  // factor. If none, another pass will be run using kDefaultDensity.
+  if (!url.is_valid()) {
+    url = FindBestMatchingIcon(icons, device_scale_factor);
+    if (!url.is_valid())

[gone, 2014/09/24 18:17:24]

may as well move this if outside of the other if since it's a fallback

[mlamouri (slow - plz ping), 2014/09/24 18:36:49]

Done.

+      url = FindBestMatchingIcon(icons, Manifest::Icon::kDefaultDensity);
+  }
+
+  return url;
+}
+
 void ShortcutHelper::OnDidGetManifest(const content::Manifest& manifest) {
   // Set the title based on the manifest value, if any.
   if (!manifest.short_name.is_null())
@@ -128,6 +235,16 @@ void ShortcutHelper::OnDidGetManifest(const content::Manifest& manifest) {
       orientation_ = manifest.orientation;
   }
 
+  GURL icon_src = FindBestMatchingIcon(manifest.icons);
+  if (icon_src.is_valid()) {
+    web_contents()->DownloadImage(icon_src,
+                                  false,
+                                  preferred_icon_size_in_px_,
+                                  base::Bind(&ShortcutHelper::OnDidDownloadIcon,
+                                             weak_ptr_factory_.GetWeakPtr()));
+    manifest_icon_status_ = MANIFEST_ICON_STATUS_FETCHING;
+  }
+
   // The ShortcutHelper is now able to notify its Java counterpart that it is
   // initialized. OnInitialized method is not conceptually part of getting the
   // manifest data but it happens that the initialization is finalized when
@@ -140,6 +257,40 @@ void ShortcutHelper::OnDidGetManifest(const content::Manifest& manifest) {
   Java_ShortcutHelper_onInitialized(env, j_obj.obj(), j_title.obj());
 }
 
+void ShortcutHelper::OnDidDownloadIcon(int id,
+                                       int http_status_code,
+                                       const GURL& url,
+                                       const std::vector<SkBitmap>& bitmaps,
+                                       const std::vector<gfx::Size>& sizes) {
+  // If getting the candidate manifest icon failed, the ShortcutHelper should
+  // fallback to the favicon.
+  // If the user already requested to add the shortcut, it will do so but use
+  // the favicon instead.
+  // Otherwise, it sets the state as if there was no manifest icon pending.
+  if (bitmaps.empty()) {
+    if (add_shortcut_requested_)
+      AddShortcutUsingFavicon();
+    else
+      manifest_icon_status_ = MANIFEST_ICON_STATUS_NONE;
+    return;
+  }
+
+  // There might be multiple bitmaps returned. The one to pick is bigger or
+  // equal to the preferred size. |bitmaps| is ordered from bigger to smaller.
+  int preferred_bitmap_index = 0;
+  for (size_t i = 0; i < bitmaps.size(); ++i) {
+    if (bitmaps[i].height() < preferred_icon_size_in_px_)
+      break;
+    preferred_bitmap_index = i;
+  }
+
+  manifest_icon_ = bitmaps[preferred_bitmap_index];
+  manifest_icon_status_ = MANIFEST_ICON_STATUS_DONE;
+
+  if (add_shortcut_requested_)
+    AddShortcutUsingManifestIcon();

[gone, 2014/09/24 18:17:24]

what happens if a shortcut wasn't requested? what uses the manifest_icon_ in
that case?

[mlamouri (slow - plz ping), 2014/09/24 18:36:49]

If the shortcut wasn't requested but is later requested, the same code path will
be taken (ie. AddShortcutUsingManifestIcon() will be called).
If the shortcut wasn't requested and is never requested, the object will be
destroyed.

+}
+
 void ShortcutHelper::TearDown(JNIEnv*, jobject) {
   Destroy();
 }
@@ -153,10 +304,43 @@ void ShortcutHelper::AddShortcut(
     jobject obj,
     jstring jtitle,
     jint launcher_large_icon_size) {
+  add_shortcut_requested_ = true;
+
   base::string16 title = base::android::ConvertJavaStringToUTF16(env, jtitle);
   if (!title.empty())
     title_ = title;
 
+  switch (manifest_icon_status_) {

[gone, 2014/09/24 18:17:24]

indentation seems off here

[mlamouri (slow - plz ping), 2014/09/24 18:36:49]

How so?

[gone, 2014/09/24 18:39:21]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Loops_and_Swi...

case should be indented by 2

[mlamouri (slow - plz ping), 2014/09/24 19:41:38]

I got hit again by Blink and Chromium having different coding style. Fixed.

+  case MANIFEST_ICON_STATUS_NONE:
+    AddShortcutUsingFavicon();
+    break;
+  case MANIFEST_ICON_STATUS_FETCHING:
+    // ::OnDidDownloadIcon() will call AddShortcutUsingManifestIcon() when run.
+    break;
+  case MANIFEST_ICON_STATUS_DONE:
+    AddShortcutUsingManifestIcon();
+    break;
+  }
+}
+
+void ShortcutHelper::AddShortcutUsingManifestIcon() {
+  // Stop observing so we don't get destroyed while doing the last steps.
+  Observe(NULL);
+
+  base::WorkerPool::PostTask(
+      FROM_HERE,
+      base::Bind(&ShortcutHelper::AddShortcutInBackgroundWithSkBitmap,
+                 url_,
+                 title_,
+                 display_,
+                 manifest_icon_,
+                 orientation_),
+      true);
+
+  Destroy();
+}
+
+void ShortcutHelper::AddShortcutUsingFavicon() {
   Profile* profile =
       Profile::FromBrowserContext(web_contents()->GetBrowserContext());
 
@@ -172,28 +356,26 @@ void ShortcutHelper::AddShortcut(
 
   // Using favicon if its size is not smaller than platform required size,
   // otherwise using the largest icon among all avaliable icons.
-  int threshold_to_get_any_largest_icon = launcher_large_icon_size_ - 1;
+  int threshold_to_get_any_largest_icon = preferred_icon_size_in_px_ - 1;
   favicon_service->GetLargestRawFaviconForPageURL(url_, icon_types,
       threshold_to_get_any_largest_icon,
-      base::Bind(&ShortcutHelper::FinishAddingShortcut,
+      base::Bind(&ShortcutHelper::OnDidGetFavicon,
                  base::Unretained(this)),
       &cancelable_task_tracker_);
 }
 
-void ShortcutHelper::FinishAddingShortcut(
+void ShortcutHelper::OnDidGetFavicon(
     const favicon_base::FaviconRawBitmapResult& bitmap_result) {
-  icon_ = bitmap_result;
-
   // Stop observing so we don't get destroyed while doing the last steps.
   Observe(NULL);
 
   base::WorkerPool::PostTask(
       FROM_HERE,
-      base::Bind(&ShortcutHelper::AddShortcutInBackground,
+      base::Bind(&ShortcutHelper::AddShortcutInBackgroundWithRawBitmap,
                  url_,
                  title_,
                  display_,
-                 icon_,
+                 bitmap_result,
                  orientation_),
       true);
 
@@ -220,7 +402,7 @@ bool ShortcutHelper::RegisterShortcutHelper(JNIEnv* env) {
   return RegisterNativesImpl(env);
 }
 
-void ShortcutHelper::AddShortcutInBackground(
+void ShortcutHelper::AddShortcutInBackgroundWithRawBitmap(
     const GURL& url,
     const base::string16& title,
     content::Manifest::DisplayMode display,
@@ -228,16 +410,26 @@ void ShortcutHelper::AddShortcutInBackground(
     blink::WebScreenOrientationLockType orientation) {
   DCHECK(base::WorkerPool::RunsTasksOnCurrentThread());
 
-  // Grab the average color from the bitmap.
-  SkColor color = SK_ColorWHITE;
-  SkBitmap favicon_bitmap;
+  SkBitmap icon_bitmap;
   if (bitmap_result.is_valid()) {
-    if (gfx::PNGCodec::Decode(bitmap_result.bitmap_data->front(),
-                              bitmap_result.bitmap_data->size(),
-                              &favicon_bitmap))
-      color = color_utils::CalculateKMeanColorOfBitmap(favicon_bitmap);
+    gfx::PNGCodec::Decode(bitmap_result.bitmap_data->front(),
+                          bitmap_result.bitmap_data->size(),
+                          &icon_bitmap);
   }
 
+  AddShortcutInBackgroundWithSkBitmap(
+      url, title, display, icon_bitmap, orientation);
+}
+
+void ShortcutHelper::AddShortcutInBackgroundWithSkBitmap(
+    const GURL& url,
+    const base::string16& title,
+    content::Manifest::DisplayMode display,
+    const SkBitmap& icon_bitmap,
+    blink::WebScreenOrientationLockType orientation) {
+  DCHECK(base::WorkerPool::RunsTasksOnCurrentThread());
+
+  SkColor color = color_utils::CalculateKMeanColorOfBitmap(icon_bitmap);
   int r_value = SkColorGetR(color);
   int g_value = SkColorGetG(color);
   int b_value = SkColorGetB(color);
@@ -249,8 +441,8 @@ void ShortcutHelper::AddShortcutInBackground(
   ScopedJavaLocalRef<jstring> java_title =
       base::android::ConvertUTF16ToJavaString(env, title);
   ScopedJavaLocalRef<jobject> java_bitmap;
-  if (favicon_bitmap.getSize())
-    java_bitmap = gfx::ConvertToJavaBitmap(&favicon_bitmap);
+  if (icon_bitmap.getSize())
+    java_bitmap = gfx::ConvertToJavaBitmap(&icon_bitmap);
 
   Java_ShortcutHelper_addShortcut(
       env,