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..d0534a70cec9c847601d7b764cc68a361e871822
--- /dev/null
+++ b/chrome/browser/manifest/manifest_icon_downloader.cc
@@ -0,0 +1,87 @@
+// 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),
+      weak_ptr_factory_(this) {
+}
+
+bool ManifestIconDownloader::Download(
+    const GURL& icon_url,
+    int ideal_icon_size_in_dp,
+    const ManifestIconDownloader::Callback& callback) {
+  content::WebContents* contents = web_contents();

[mlamouri (slow - plz ping), 2015/08/05 08:20:16]

nit: no need for that, web_contents() is an accessor from the object.

[Lalit Maganti, 2015/08/05 14:47:13]

Done.

+  if (!contents || !icon_url.is_valid()) return false;
+
+  contents->DownloadImage(
+      icon_url,
+      false,
+      0,
+      false,
+      base::Bind(&ManifestIconDownloader::OnIconFetched,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 ideal_icon_size,
+                 callback));
+  return true;
+}
+
+void ManifestIconDownloader::OnIconFetched(

[mlamouri (slow - plz ping), 2015/08/05 08:20:16]

It would be great to have unit tests for this method.

[Lalit Maganti, 2015/08/05 14:47:13]

Added.

+    int ideal_icon_size_in_dp,
+    const ManifestIconDownloader::Callback& callback,
+    int id,
+    int http_status_code,
+    const GURL& url,
+    const std::vector<SkBitmap>& bitmaps,
+    const std::vector<gfx::Size>& sizes) {
+  content::WebContents* contents = web_contents();

[mlamouri (slow - plz ping), 2015/08/05 08:20:16]

ditto

[Lalit Maganti, 2015/08/05 14:47:13]

Done.

+  if (!contents || bitmaps.empty()) {
+    callback.Run(SkBitmap());
+    return;
+  }
+
+  const gfx::Screen* screen =
+      gfx::Screen::GetScreenFor(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;
+
+  // This algorithm attempts to find an icon equal to the size we want or,
+  // failing that, an icon which is as close to our ideal size which we can

[gone, 2015/08/04 21:41:45]

Other reviewers are very pretty nit-picky about comments having "we" in them. 
The comment also seems wrong -- you do an early return instead of scaling when
you find an icon that's the ideal size.

How about:
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 are
assumed to be square-shaped -- only the height is checked.

[Lalit Maganti, 2015/08/05 14:47:13]

Done.

+  // scale down to our required size. We should hopefully have a URL provided
+  // to us which should be an icon close in size anyway, but we scale regardless
+  // of how big the icon is.
+  // This entire algorithm assumes square or very close to square icons.
+  const SkBitmap* best_upper = nullptr;
+  for (size_t i = 0; i < bitmaps.size(); ++i) {
+    const SkBitmap& current_bitmap = bitmaps[i];
+    int height = current_bitmap.height();
+
+    if (height == ideal_icon_size_in_px) {

[mlamouri (slow - plz ping), 2015/08/05 08:20:16]

This is assuming height === width, right? Shouldn't you first ignore bitmaps
with height != width?

[Lalit Maganti, 2015/08/05 14:47:13]

Done.

+      callback.Run(current_bitmap);
+      return;
+    }
+
+    if (height > ideal_icon_size_in_px &&
+        (!best_upper || height < best_upper->height())) {
+      best_upper = &current_bitmap;
+    }
+  }
+
+  if (best_upper) {

[mlamouri (slow - plz ping), 2015/08/05 08:20:16]

That's me or you don't actually close that { ?

Though, I would handle the error case first:

if (!best_upper) {
  callback.Run(SkBitMap());
  return;
}

[...]

[Lalit Maganti, 2015/08/05 14:47:13]

Done.

+    callback.Run(skia::ImageOperations::Resize(
+            image, skia::ImageOperations::RESIZE_BEST,
+            ideal_icon_size_in_px,
+            ideal_icon_size_in_px));
+  else
+    callback.Run(SkBitmap());
+}