Use hi-resolution favicon variants if available.

Use hi-resolution favicon variants if available.

This changes the favicon IPC to return all favicon variants to the browser,
and hands the to the new in-browser variant selection function.

Note that favicons that come from the history database will still
be lodpi. Reloading a site will force the favicon to be loaded from
the network. (chrome:// favicons are always loaded through the favicon
cache and hence are always lodpi)

BUG=138550
TEST=Go to apple.com, amazon.com, github.com, notice hidpi favicon

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150184

[pkotwicz, 2012-08-02 23:47:08]

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/browser/favi...
File chrome/browser/favicon/favicon_handler.cc (right):

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/browser/favi...
chrome/browser/favicon/favicon_handler.cc:523: sizes.push_back(2 * image_size); 
// XXX mac-specific
Can you use ui::GetSupportedScaleFactors here instead. We will probably end up
using something different than ui::GetSupportedScaleFactors. But
ui::GetSupportedScaleFactors is slightly better than this.

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/browser/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/browser/favi...
chrome/browser/favicon/favicon_tab_helper.cc:207: const std::vector<SkBitmap>&
image) {
Please name this parameter bitmaps

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/browser/favi...
chrome/browser/favicon/favicon_tab_helper.cc:215: image[i].width() / 16.0))); 
// XXX
16 -> gfx::kFaviconSize.
Probably going to be some work here, once we want gfx::kFaviconSize to be 24
instead.

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/browser/favi...
File chrome/browser/favicon/favicon_util.cc (right):

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/browser/favi...
chrome/browser/favicon/favicon_util.cc:14: std::vector<int> image_sizes) {
bitmap_sizes

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/renderer/chr...
File chrome/renderer/chrome_render_view_observer.cc (right):

https://chromiumcodereview.appspot.com/10828127/diff/4002/chrome/renderer/chr...
chrome/renderer/chrome_render_view_observer.cc:1063: const
std::vector<SkBitmap>& images) {
Nit: bitmaps

https://chromiumcodereview.appspot.com/10828127/diff/4002/webkit/glue/image_d...
File webkit/glue/image_decoder.cc (right):

https://chromiumcodereview.appspot.com/10828127/diff/4002/webkit/glue/image_d...
webkit/glue/image_decoder.cc:41: const WebKit::WebVector<WebImage>& images =
WebImage::framesFromData(
Nit: images -> bitmaps

[Nico, 2012-08-02 23:49:30]

Thanks for the comments. This is currently just a wip patch. I'll slice it into
several patches for review. I'll address your comments before sending out the
pieces.

[Nico, 2012-08-06 20:29:19]

Alright gentlemen, this is it. I think it came up pretty clear, except for one
somewhat ugly part, called out below.

(And as the TODO in that area mentions, we might want to move that code into
FaviconHandler when we make the favicon cache hidpi-aware).

https://chromiumcodereview.appspot.com/10828127/diff/17020/chrome/browser/fav...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/10828127/diff/17020/chrome/browser/fav...
chrome/browser/favicon/favicon_tab_helper.cc:204:
scale_factors.push_back(ui::SCALE_FACTOR_100P);  // TODO: Aura?
pkotwicz: How should this be done?

[Nico, 2012-08-06 20:29:53]

Hm, rietveld decided to omit the CL description. Here it is: """

Use hi-resolution favicon variants if available.

This changes the favicon IPC to return all favicon variants to the browser,
and hands the to the new in-browser variant selection function.

Note that favicons that come from the history database will still
be lodpi. Reloading a site will force the favicon to be loaded from
the network. (chrome:// favicons are always loaded through the favicon
cache and hence are always lodpi)

BUG=138550
TEST=Go to apple.com, amazon.com, github.com, notice hidpi favicon

"""

On Mon, Aug 6, 2012 at 1:29 PM,  <thakis@chromium.org> wrote:
> Alright gentlemen, this is it. I think it came up pretty clear, except for
> one
> somewhat ugly part, called out below.
>
> (And as the TODO in that area mentions, we might want to move that code into
> FaviconHandler when we make the favicon cache hidpi-aware).
>
>
>
https://chromiumcodereview.appspot.com/10828127/diff/17020/chrome/browser/fav...
> File chrome/browser/favicon/favicon_tab_helper.cc (right):
>
>
https://chromiumcodereview.appspot.com/10828127/diff/17020/chrome/browser/fav...
> chrome/browser/favicon/favicon_tab_helper.cc:204:
> scale_factors.push_back(ui::SCALE_FACTOR_100P);  // TODO: Aura?
> pkotwicz: How should this be done?
>
> https://chromiumcodereview.appspot.com/10828127/

[sky, 2012-08-06 20:49:14]

LGTM

http://codereview.chromium.org/10828127/diff/17020/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

http://codereview.chromium.org/10828127/diff/17020/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_tab_helper.cc:206: gfx::Image
favicon(SelectFaviconFrames(bitmaps, scale_factors, requested_size));
nit: > 80

[jamesr, 2012-08-06 20:59:25]

webkit/ lgtm

[commit-bot: I haz the power, 2012-08-06 21:00:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10828127/16004

[commit-bot: I haz the power, 2012-08-06 22:17:23]

The commit queue went berserk retrying too often for a
seemingly flaky test. Builder is linux_chromeos, revision is 150142, job name
was 10828127-16004 (previous was lost) (previous was lost) (previous was lost)
(retry).

[commit-bot: I haz the power, 2012-08-06 22:24:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10828127/13032

[commit-bot: I haz the power, 2012-08-06 22:47:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10828127/3040

[commit-bot: I haz the power, 2012-08-06 23:23:49]

Try job failure for 10828127-3040 (retry) on linux_chromeos for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2012-08-06 23:27:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/10828127/15024