Fix favicon exact match logic and add test.

Fix favicon exact match logic and add test.


BUG=119437
TEST=unit_tests pass + see issue.


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

[stevenjb, 2012-03-24 19:07:55]

http://codereview.chromium.org/9852012/diff/1/chrome/browser/favicon/favicon_...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/9852012/diff/1/chrome/browser/favicon/favicon_...
chrome/browser/favicon/favicon_handler.cc:56: return a.substr(0, a.rfind('#'))
==  b.substr(0, b.rfind('#'));
This addresses a pre-existing inconsistency in favicon behavior that I
discovered while testing. Sites like GMail use fragments, and a complete favicon
update does not appear to occur when just the fragment changes.

http://codereview.chromium.org/9852012/diff/1/chrome/browser/favicon/favicon_...
chrome/browser/favicon/favicon_handler.cc:173: update_candidate = true;
The real bug was that exact_match was not getting set correctly here.

[sky, 2012-03-25 03:59:23]

http://codereview.chromium.org/9852012/diff/1/chrome/browser/favicon/favicon_...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/9852012/diff/1/chrome/browser/favicon/favicon_...
chrome/browser/favicon/favicon_handler.cc:56: return a.substr(0, a.rfind('#'))
==  b.substr(0, b.rfind('#'));
On 2012/03/24 19:07:56, stevenjb (chromium) wrote:
> This addresses a pre-existing inconsistency in favicon behavior that I
> discovered while testing. Sites like GMail use fragments, and a complete
favicon
> update does not appear to occur when just the fragment changes.

This makes me nervous as I'm not sure of the rules of GURL parsing. Doesn't GURL
have a way to get the fragment directly?

[stevenjb, 2012-03-25 18:38:22]

http://codereview.chromium.org/9852012/diff/1/chrome/browser/favicon/favicon_...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/9852012/diff/1/chrome/browser/favicon/favicon_...
chrome/browser/favicon/favicon_handler.cc:56: return a.substr(0, a.rfind('#'))
==  b.substr(0, b.rfind('#'));
On 2012/03/25 03:59:23, sky wrote:
> On 2012/03/24 19:07:56, stevenjb (chromium) wrote:
> > This addresses a pre-existing inconsistency in favicon behavior that I
> > discovered while testing. Sites like GMail use fragments, and a complete
> favicon
> > update does not appear to occur when just the fragment changes.
> 
> This makes me nervous as I'm not sure of the rules of GURL parsing. Doesn't
GURL
> have a way to get the fragment directly?
Good point, trimming gurl.ref() will be much more robust.
Done.

[sky, 2012-03-25 18:46:46]

http://codereview.chromium.org/9852012/diff/3001/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/9852012/diff/3001/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:53: std::string url = gurl.spec();
I believe you want ReplaceComponents.

[stevenjb, 2012-03-25 22:00:15]

http://codereview.chromium.org/9852012/diff/3001/chrome/browser/favicon/favic...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/9852012/diff/3001/chrome/browser/favicon/favic...
chrome/browser/favicon/favicon_handler.cc:53: std::string url = gurl.spec();
On 2012/03/25 18:46:46, sky wrote:
> I believe you want ReplaceComponents.
So I do, thanks.
Done.

[sky, 2012-03-25 22:48:39]

LGTM

[commit-bot: I haz the power, 2012-03-26 00:13:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stevenjb@chromium.org/9852012/3003

[commit-bot: I haz the power, 2012-03-26 01:43:14]

Change committed as 128851