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),
+      weak_ptr_factory_(this) {
+}
+
+bool ManifestIconDownloader::Download(
+    const GURL& icon_url,
+    int ideal_icon_size,
+    const ManifestIconDownloader::Callback& callback) {
+  content::WebContents* contents = web_contents();
+  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(
+    int ideal_icon_size,

[gone, 2015/07/31 18:09:20]

rename this ideal_icon_size_in_units (dp?)

[Lalit Maganti, 2015/08/03 08:58:29]

Will do.

+    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();
+  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 scaled_ideal_icon_size = ideal_icon_size * device_scale_factor;

[gone, 2015/07/31 18:09:20]

rename this so that the unit is obvious

[Lalit Maganti, 2015/08/03 08:58:29]

Will do.

+
+  const SkBitmap* best_upper = nullptr;

[gone, 2015/07/31 18:09:20]

best upper what?  Document everything about the selection algorithm you're
using, especially since it seems to be heuristic and different from the one in
ManifestIconSelector::FindBestMatchingIcon (which documents all the things).

[Lalit Maganti, 2015/08/03 08:58:29]

Will do.

+  const SkBitmap* best_lower = nullptr;
+
+  // This entire algorithm assumes square or very close to square icons.
+  for (size_t i = 0; i < bitmaps.size(); ++i) {
+    const SkBitmap& current_bitmap = bitmaps[i];
+    int height = current_bitmap.height();
+
+    if (height == scaled_ideal_icon_size) {
+      callback.Run(current_bitmap);
+      return;
+    }
+
+    if (height > scaled_ideal_icon_size &&
+        (!best_upper || height < best_upper->height())) {
+      best_upper = &current_bitmap;
+    } else if (height < scaled_ideal_icon_size &&
+        (!best_lower || height > best_lower->height())) {
+      best_lower = &current_bitmap;
+    }
+  }
+
+  float soft_upper_bound = 1.5 * scaled_ideal_icon_size;

[gone, 2015/07/31 18:09:20]

How'd you come up with these bounds?  2/1 and 1/2 I can I reason about, but 3/2
and 9/10?

[Lalit Maganti, 2015/08/03 08:58:29]

They are completely arbitary - I just put them in as placeholders for now.

[gone, 2015/08/03 23:27:58]

I'd argue that having anything larger that you can scale down is better than
capping it at 2x.

[Lalit Maganti, 2015/08/04 08:59:53]

Right OK.

+  float soft_lower_bound = 0.9 * scaled_ideal_icon_size;
+
+  float hard_upper_bound = 2.0 * scaled_ideal_icon_size;
+  float hard_lower_bound = 0.5 * scaled_ideal_icon_size;
+
+  if (best_upper && best_upper->height() < soft_upper_bound) {

[gone, 2015/07/31 18:09:20]

I don't think it ever makes sense to scale up if you have a higher quality icon
that you can scale down.

[Lalit Maganti, 2015/08/03 08:58:29]

Hmmm this is an interesting one. I was thinking along the same lines but then I
also think that if you have an icon which is just a bit smaller than required
(say 95%), it may be better to scale it up rather than scaling down a 200% icon.
I'm not really sure if this is correct thinking though - I've never really
looked into image scaling too deeply.

[gone, 2015/08/03 23:27:58]

It is actually always preferable to scale down a 200% over scaling up a 95% icon
because the pixels line up.  They don't when you scale up a 95% icon, resulting
in extra fuzziness.

But yeah, that's the reason the original code ignored anything too small.  I've
asked multiple folks here in the office and we're all in agreement about never
scaling up.  The problem is that scaling down means you have an extra amount of
information you can use to determine what a downscaled image's pixel would look
like -- with upscaling you have to guess about what should have been in that
spot.

The wikipedia article on image scaling is actually worth a read:
https://en.wikipedia.org/wiki/Image_scaling

We'd considered using complicated algorithms at some point to scale tiny
favicons up to look like app icons, but the quality was terrible.

[Lalit Maganti, 2015/08/04 08:59:53]

OK that sounds good to me - I can vastly simplify this code and just pick the
exact or closest one higher then and get rid of all the scale up code.

+    ScaleAndCallback(*best_upper, scaled_ideal_icon_size, callback);
+  } else if (best_lower && best_lower->height() > soft_lower_bound) {
+    ScaleAndCallback(*best_lower, scaled_ideal_icon_size, callback);
+  } else if (best_upper && best_upper->height() < hard_upper_bound) {
+    ScaleAndCallback(*best_upper, scaled_ideal_icon_size, callback);
+  } else if (best_lower && best_lower->height() > hard_lower_bound) {
+    ScaleAndCallback(*best_lower, scaled_ideal_icon_size, callback);
+  } else {
+    callback.Run(SkBitmap());
+  }
+}
+
+void ManifestIconDownloader::ScaleAndCallback(
+    const SkBitmap& image,
+    const float scaled_ideal_icon_size,
+    const Callback& callback) {
+  callback.Run(skia::ImageOperations::Resize(
+          image, skia::ImageOperations::RESIZE_BEST,
+          scaled_ideal_icon_size,
+          scaled_ideal_icon_size));
+}