(Moved) Refactor SelectFaviconFrames to make it easy to use SelectFaviconFrameson

Refactor SelectFaviconFrames to make it easy to use SelectFaviconFrames on objects other than SkBitmaps.
Changed the selection and score calculation code to operate on std::vector<gfx::Size> instead of an std::vector<SkBitmap>.

Bug=10802066
Test=SelectFaviconFramesTest.* pass

[pkotwicz, 2012-08-27 17:12:47]

Refactor SelectFaviconFrames to make it easy to use SelectFaviconFrames on
objects other than SkBitmaps.
Changed the selection and score calculation code to operate on
std::vector<gfx::Size> instead of std::vector<SkBitmap>.

My goal in this patch is to allow the history service to use
SelectFaviconFrames, but on non bitmap data it receives from the thumbnail
database. Take a look at http://codereview.chromium.org/10802066/ to see how I
use it.

I am hoping to move select_favicon_frames to history/ but will do that in a
separate patch.

[tfarina, 2012-08-27 17:18:36]

weird, rietveld shows the file as deleted. :-/

[pkotwicz, 2012-08-27 17:31:23]

tfarina, I think that the reason it shows the file as deleted is that the file
was moved in Patch Set 1 but its not anymore. (I think it's a reitveld bug). I
will however create a new issue before hitting the CQ though.

[Nico, 2012-08-27 19:35:44]

This looks great!

Weird that rietveld thinks the file got deleted; the diff shows up fine.

http://codereview.chromium.org/10883056/diff/6001/chrome/browser/favicon/sele...
File chrome/browser/favicon/select_favicon_frames.cc (right):

http://codereview.chromium.org/10883056/diff/6001/chrome/browser/favicon/sele...
chrome/browser/favicon/select_favicon_frames.cc:18: DCHECK(sizes);
No need, the clear() / push_back() will blow up immediately if this is NULL.

http://codereview.chromium.org/10883056/diff/6001/chrome/browser/favicon/sele...
chrome/browser/favicon/select_favicon_frames.cc:19: sizes->clear();
nit: I'd omit this too. I find it clearer and more flexible to do this at the
calling site if needed

http://codereview.chromium.org/10883056/diff/6001/chrome/browser/favicon/sele...
chrome/browser/favicon/select_favicon_frames.cc:170:
std::vector<SelectionResult>* results) {
(You don't DCHECK / clear |results| either)

http://codereview.chromium.org/10883056/diff/6001/chrome/browser/favicon/sele...
chrome/browser/favicon/select_favicon_frames.cc:216: for (size_t i = 0; i <
results.size(); ++i) {
style nit: It helps readability if every action in a function is roughly at the
same abstraction level. The first 5 lines in this function are at a fairly high
level, and this loop is pretty low-level. Maybe move the for loop body into its
own function ("ApplyResizeMethod()"?)

http://codereview.chromium.org/10883056/diff/6001/chrome/browser/favicon/sele...
chrome/browser/favicon/select_favicon_frames.cc:237: default:
Say `case NONE:` here, then compilers will warn if the ResizeMethod enum grows
another entry and the switch isn't updated. (Also, your indent is off by 1 here)

[pkotwicz, 2012-08-27 20:00:47]

Changes as requested

[Nico, 2012-08-27 20:12:35]

lgtm

[pkotwicz, 2012-08-27 20:33:33]

Moving the CL to codereview.chromium.org/10872093 because of reitveld weirdness.