Implement manifest icon downloader

chrome/browser/manifest/manifest_icon_downloader.cc

+// 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/web_contents.h"
+#include "skia/ext/image_operations.h"
+#include "ui/gfx/screen.h"
+
+ManifestIconDownloader::ManifestIconDownloader(
+    content::WebContents* web_contents)
+    : WebContentsObserver(web_contents) {
+}
+
+bool ManifestIconDownloader::Download(
+    const GURL& icon_url,
+    int ideal_icon_size_in_dp,
+    const ManifestIconDownloader::IconFetchCallback& callback) {
+  if (!web_contents() || !icon_url.is_valid()) return false;
+
+  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;
+
+  web_contents()->DownloadImage(
+      icon_url,
+      false,
+      0,
+      false,

[mlamouri (slow - plz ping), 2015/08/07 10:11:11]

Could you add comments after the three arguments above to say what they mean?
|icon_url| is clear but I have no idea what |false| or 0 mean here.

[Lalit Maganti, 2015/08/07 13:04:30]

Done.

+      base::Bind(&ManifestIconDownloader::OnIconFetched,
+                 ideal_icon_size_in_px,
+                 callback));
+  return true;
+}
+
+void ManifestIconDownloader::OnIconFetched(
+    int ideal_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) {
+  const int closest_index = FindClosestBitmapIndex(
+      ideal_icon_size_in_px, bitmaps);
+
+  if (closest_index == -1) {
+    callback.Run(SkBitmap());
+    return;
+  }
+
+  const SkBitmap& scaled = skia::ImageOperations::Resize(

[mlamouri (slow - plz ping), 2015/08/07 10:11:11]

We need to make sure this happens in the IO thread. Could you add:
  DCHECK_CURRENTLY_ON(content::BrowserThread::IO);
at the beginning of that method?

Also, shouldn't we only call that method if there is a difference between ideal
and what we got?

[Lalit Maganti, 2015/08/07 13:04:30]

Refactored to make resizing happen in background.

+      bitmaps[closest_index],
+      skia::ImageOperations::RESIZE_BEST,
+      ideal_icon_size_in_px,
+      ideal_icon_size_in_px);
+  callback.Run(scaled);
+}
+
+int ManifestIconDownloader::FindClosestBitmapIndex(
+    int ideal_icon_size_in_px,
+    const std::vector<SkBitmap>& bitmaps) {
+  if (bitmaps.empty())
+    return -1;
+
+  // This algorithm searches for an icon equal to the ideal size.  Failing that,
+  // it picks the smallest icon larger than the ideal size to downscale.  Icons
+  // which are not in a square shape are ignored.
+  const SkBitmap* best_upper = nullptr;
+  int best_upper_index = -1;
+
+  for (size_t i = 0; i < bitmaps.size(); ++i) {

[mlamouri (slow - plz ping), 2015/08/07 10:11:11]

Let's use some C++11 syntax sugar:
for (const auto& bitmap : bitmaps) {
  [...]
}

[Lalit Maganti, 2015/08/07 13:04:30]

I would if I could but I need the index to return :(

+    const SkBitmap& current_bitmap = bitmaps[i];
+    int height = current_bitmap.height();

[mlamouri (slow - plz ping), 2015/08/07 10:11:11]

no need to have a temporary variable here.

[Lalit Maganti, 2015/08/07 13:04:30]

Done.

+    if (height != current_bitmap.width())
+      continue;
+
+    if (height == ideal_icon_size_in_px)
+      return i;
+
+    if (height > ideal_icon_size_in_px &&
+        (!best_upper || height < best_upper->height())) {
+      best_upper = &current_bitmap;
+      best_upper_index = i;
+    }
+  }
+
+  return best_upper_index;
+}