Give SelectFaviconFrames a quality score.

Give SelectFaviconFrames a quality score.

Use this score to decide which favicon to pick.

BUG=141039
TEST=visit reddit.com. Favicon shows just the head of the reddit
alient, not the whole alien.

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

[pkotwicz, 2012-08-15 22:29:09]

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

https://chromiumcodereview.appspot.com/10824319/diff/1001/chrome/browser/favi...
chrome/browser/favicon/favicon_handler.cc:170: bool exact_match = score == 0;
I might be misunderstanding, but shouldn't this check score == 1

https://chromiumcodereview.appspot.com/10824319/diff/1001/chrome/browser/favi...
File chrome/browser/favicon/select_favicon_frames.cc (right):

https://chromiumcodereview.appspot.com/10824319/diff/1001/chrome/browser/favi...
chrome/browser/favicon/select_favicon_frames.cc:147: *match_score = 0.8;
Shouldn't this be a constant?

[Nico, 2012-08-15 22:30:43]

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

https://chromiumcodereview.appspot.com/10824319/diff/1001/chrome/browser/favi...
chrome/browser/favicon/favicon_handler.cc:170: bool exact_match = score == 0;
On 2012/08/15 22:29:09, pkotwicz wrote:
> I might be misunderstanding, but shouldn't this check score == 1

Err, yes.

[Nico, 2012-08-15 22:39:09]

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

https://chromiumcodereview.appspot.com/10824319/diff/1001/chrome/browser/favi...
chrome/browser/favicon/favicon_handler.cc:170: bool exact_match = score == 0;
On 2012/08/15 22:30:43, Nico wrote:
> On 2012/08/15 22:29:09, pkotwicz wrote:
> > I might be misunderstanding, but shouldn't this check score == 1
> 
> Err, yes.

(Done.)

[sky, 2012-08-16 00:26:20]

LGTM - can you add more coverage to FaviconIconHandlerTest for choosing the best
match?

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

https://chromiumcodereview.appspot.com/10824319/diff/7003/chrome/browser/favi...
chrome/browser/favicon/favicon_tab_helper.cc:198: float score;
Can you set this to 0 here jsut to be sure.

[Nico, 2012-08-16 01:07:54]

Thanks!

On Wed, Aug 15, 2012 at 5:26 PM,  <sky@chromium.org> wrote:
> LGTM - can you add more coverage to FaviconIconHandlerTest for choosing the
> best match?

Added another test to SelectFaviconFramesTest instead.

>
>
>
https://chromiumcodereview.appspot.com/10824319/diff/7003/chrome/browser/favi...
> File chrome/browser/favicon/favicon_tab_helper.cc (right):
>
>
https://chromiumcodereview.appspot.com/10824319/diff/7003/chrome/browser/favi...
> chrome/browser/favicon/favicon_tab_helper.cc:198: float score;
> Can you set this to 0 here jsut to be sure.
>
> https://chromiumcodereview.appspot.com/10824319/

[sky, 2012-08-16 02:36:17]

SLGTM

On Wed, Aug 15, 2012 at 6:07 PM, Nico Weber <thakis@chromium.org> wrote:
> Thanks!
>
> On Wed, Aug 15, 2012 at 5:26 PM,  <sky@chromium.org> wrote:
>> LGTM - can you add more coverage to FaviconIconHandlerTest for choosing the
>> best match?
>
> Added another test to SelectFaviconFramesTest instead.
>
>>
>>
>>
https://chromiumcodereview.appspot.com/10824319/diff/7003/chrome/browser/favi...
>> File chrome/browser/favicon/favicon_tab_helper.cc (right):
>>
>>
https://chromiumcodereview.appspot.com/10824319/diff/7003/chrome/browser/favi...
>> chrome/browser/favicon/favicon_tab_helper.cc:198: float score;
>> Can you set this to 0 here jsut to be sure.
>>
>> https://chromiumcodereview.appspot.com/10824319/