Cleanup FaviconHandler

Cleanup FaviconHandler code now that http://codereview.chromium.org/10870022/ has landed.

Fix FaviconHandler::DownloadImage to return the best matching unresized bitmap as specified in favicon_tab_helper.h

Bug=130896
Test=FaviconHandlerTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156222

[rjkroege, 2012-09-07 16:45:46]

http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:88: bool HasExpiredResult(
why change the name if the function is the same?

http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:250: SetHistoryFavicon(url, icon_url,
image_data, icon_type);
when do we actually get a sized version? Have you  moved this to the paint side?

http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.h (right):

http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.h:117: // |image| is resized to how it
should appear in the UI and be stored in
Where is the image resized?

[pkotwicz, 2012-09-07 17:25:39]

http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:250: SetHistoryFavicon(url, icon_url,
image_data, icon_type);
The resizing occurs in FaviconTabHelper::OnDidDownloadFavicon
I believe the code which does the resizing here is currently inactive as this
function gets called with an already resized image, barring the case of
preferred_icon_size == 0

[rjkroege, 2012-09-07 19:41:26]

On 2012/09/07 17:25:39, pkotwicz wrote:
>
http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
> File chrome/browser/favicon/favicon_handler.cc (right):
> 
>
http://codereview.chromium.org/10911149/diff/2001/chrome/browser/favicon/favi...
> chrome/browser/favicon/favicon_handler.cc:250: SetHistoryFavicon(url,
icon_url,
> image_data, icon_type);
> The resizing occurs in FaviconTabHelper::OnDidDownloadFavicon
> I believe the code which does the resizing here is currently inactive as this
> function gets called with an already resized image, barring the case of
> preferred_icon_size == 0

Is there a test that exercises this?

[pkotwicz, 2012-09-09 22:03:18]

Fixed FaviconHandler::DownloadImage() to return an unresized image in callback
as specified in favicon_tab_helper.h
Added unittest to test that the callback returns an unresized image.

[rjkroege, 2012-09-10 15:57:07]

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:99: bool HasValidResult(
You have several very similar loops. Your code could be way shorter is you used
the stl's find_if.

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:368: float score = 0.0f;
we don't ever seem to use this value again except below in the image
constructor. Suggests to me that SelectFaviconFrames should not take the
parameter.

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.h (right):

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.h:115: void OnDidDownloadFavicon(int id,
nit: My preference would be for this to have a comment saying what it does. But
it was like that before.

[pkotwicz, 2012-09-10 17:34:38]

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:99: bool HasValidResult(
On 2012/09/10 15:57:08, rjkroege wrote:
> You have several very similar loops. Your code could be way shorter is you
used
> the stl's find_if.

Done.

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:368: float score = 0.0f;
I am unsure which parameter you are referring to.
It is true that |scale_factors| is not used again. And I think that eventually
SelectFaviconFrames() should take a vector of gfx::Size as opposed to both
requested_size and |scale_factors|. There is a TODO in SelectFaviconFrames() to
this effect. However, I will do this in a separate CL as it changes how favicons
are resized.
|score| is used again in UpdateFaviconCandidate().

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.h (right):

http://codereview.chromium.org/10911149/diff/8001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.h:115: void OnDidDownloadFavicon(int id,
On 2012/09/10 15:57:08, rjkroege wrote:
> nit: My preference would be for this to have a comment saying what it does.
But
> it was like that before.

Done.

[rjkroege, 2012-09-11 20:04:52]

lgtm

[pkotwicz, 2012-09-11 21:16:53]

Scott can you please take a look?

[sky, 2012-09-11 23:03:58]

LGTM

[commit-bot: I haz the power, 2012-09-11 23:36:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10911149/11028

[commit-bot: I haz the power, 2012-09-12 00:39:49]

Try job failure for 10911149-11028 (retry) on win for step "runhooks" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[Nico, 2012-09-12 00:46:36]

On 2012/09/12 00:39:49, I haz the power (commit-bot) wrote:
> Try job failure for 10911149-11028 (retry) on win for step "runhooks" (clobber
> build).
> It's a second try, previously, step "compile" failed.
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Out-of-disk is a known issue with the bots at the moment. If the other bots come
back green, you can land with "NOTRY=true" in the CL description (then the cq
will just land and not run jobs)

[commit-bot: I haz the power, 2012-09-12 00:52:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/10911149/11028

[commit-bot: I haz the power, 2012-09-12 03:25:07]

Change committed as 156222