Implement manifest icon downloader

chrome/browser/manifest/manifest_icon_downloader.cc

Index: chrome/browser/manifest/manifest_icon_downloader.cc
diff --git a/chrome/browser/manifest/manifest_icon_downloader.cc b/chrome/browser/manifest/manifest_icon_downloader.cc
new file mode 100644
index 0000000000000000000000000000000000000000..1c42ac16036d92a37956634f94360d57831936ef
--- /dev/null
+++ b/chrome/browser/manifest/manifest_icon_downloader.cc
@@ -0,0 +1,122 @@
+// Copyright 2015 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/manifest/manifest_icon_downloader.h"
+
+#include "chrome/browser/manifest/manifest_icon_selector.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/web_contents.h"
+#include "skia/ext/image_operations.h"
+#include "ui/gfx/screen.h"
+
+bool ManifestIconDownloader::Download(
+    content::WebContents* web_contents,
+    const GURL& icon_url,
+    int ideal_icon_size_in_dp,
+    const ManifestIconDownloader::IconFetchCallback& callback) {
+  if (!web_contents || !icon_url.is_valid()) return false;

[mlamouri (slow - plz ping), 2015/08/21 09:27:11]

style: contrary to java, you must have |return false;| on a new line

[Lalit Maganti, 2015/08/21 12:14:19]

Done.

+
+  const gfx::Screen* screen =
+      gfx::Screen::GetScreenFor(web_contents->GetNativeView());
+
+  const float device_scale_factor =
+      screen->GetPrimaryDisplay().device_scale_factor();
+  const float ideal_icon_size_in_px =
+      ideal_icon_size_in_dp * device_scale_factor;
+
+  const int minimum_scale_factor = std::max(
+      static_cast<int>(floor(device_scale_factor - 1)), 1);
+  const float minimum_icon_size_in_px =
+      ideal_icon_size_in_dp * minimum_scale_factor;
+
+  web_contents->DownloadImage(
+      icon_url,
+      false, // is_favicon
+      0,     // max_bitmap_size - 0 means no maximum size.
+      false, // bypass_cache
+      base::Bind(&ManifestIconDownloader::OnIconFetched,
+                 ideal_icon_size_in_px,
+                 minimum_icon_size_in_px,
+                 callback));
+  return true;
+}
+
+void ManifestIconDownloader::OnIconFetched(
+    int ideal_icon_size_in_px,
+    int minimum_icon_size_in_px,
+    const ManifestIconDownloader::IconFetchCallback& callback,
+    int id,
+    int http_status_code,
+    const GURL& url,
+    const std::vector<SkBitmap>& bitmaps,
+    const std::vector<gfx::Size>& sizes) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+  const int closest_index = FindClosestBitmapIndex(

[mlamouri (slow - plz ping), 2015/08/21 09:27:11]

nit: leave a blank line between the DCHECK() and the const int

[Lalit Maganti, 2015/08/21 12:14:19]

Done.

+      ideal_icon_size_in_px, minimum_icon_size_in_px, bitmaps);
+
+  if (closest_index == -1) {
+    callback.Run(SkBitmap());
+    return;
+  }
+
+  const SkBitmap& chosen = bitmaps[closest_index];
+
+  // Only scale if we need to scale down. For scaling up we will let the system
+  // handle that when it is required to display it. This saves space in the
+  // webapp storage system as well.
+  if (chosen.height() > ideal_icon_size_in_px) {
+    content::BrowserThread::PostTask(
+        content::BrowserThread::IO,
+        FROM_HERE,
+        base::Bind(&ManifestIconDownloader::ScaleIcon,
+                   ideal_icon_size_in_px,
+                   chosen,
+                   callback));
+    return;
+  }
+
+  callback.Run(chosen);
+}
+
+void ManifestIconDownloader::ScaleIcon(
+    int ideal_icon_size_in_px,
+    const SkBitmap& bitmap,
+    const ManifestIconDownloader::IconFetchCallback& callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
+
+  const SkBitmap& scaled = skia::ImageOperations::Resize(
+      bitmap,
+      skia::ImageOperations::RESIZE_BEST,
+      ideal_icon_size_in_px,
+      ideal_icon_size_in_px);
+  content::BrowserThread::PostTask(

[mlamouri (slow - plz ping), 2015/08/21 09:27:11]

nit: I would add a blank line, up to you.

[Lalit Maganti, 2015/08/21 12:14:19]

Done.

+      content::BrowserThread::UI,
+      FROM_HERE,
+      base::Bind(callback, scaled));
+}
+
+int ManifestIconDownloader::FindClosestBitmapIndex(
+    int ideal_icon_size_in_px,
+    int minimum_icon_size_in_px,
+    const std::vector<SkBitmap>& bitmaps) {
+  // 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 best_index = -1;
+  for (size_t i = 0; i < bitmaps.size(); ++i) {
+    if (bitmaps[i].height() != bitmaps[i].width())
+      continue;
+    if (bitmaps[i].height() == ideal_icon_size_in_px)
+      return i;
+    if (bitmaps[i].height() < ideal_icon_size_in_px) {
+      // best_index == -1 means the first good icon which has been found

[gone, 2015/08/20 18:48:42]

if you have to explicitly explain what -1 means, define it as a constant.  it's
used in a few places already, IIRC, so it'd be a good thing to do anyway.

[Lalit Maganti, 2015/08/21 12:14:19]

Code has changed enough such that use of -1 is much clearer without the
constant.

+      // is smaller than the ideal size. It can still be used though if it is
+      // bigger than the minimum.

[mlamouri (slow - plz ping), 2015/08/21 09:27:11]

Hmm, if you found an icon bigger than minimum and smaller than the ideal, given
than the next one is going to be smaller, shouldn't you stop there and return
it?

Then, maybe you don't need best_index and you should just return -1 after the
loop?

[Lalit Maganti, 2015/08/21 12:14:19]

Changed as discussed offline.

+      if (best_index == -1)
+        return bitmaps[i].height() >= minimum_icon_size_in_px ? i : -1;
+      break;
+    }
+    best_index = i;
+  }
+  return best_index;
+}