Don't request missing favicon on every page request.

Don't request missing favicon on every page request. 
Mark Favicon as 'Unable to Download' if server returns HTTP status 404 and
don't try to download it again until user closes the browser or clicks 'Shift-Reload'
(RELOAD_IGNORING_CACHE). 

Firefox 20 and IE 10 don't request missing favicon.ico on every page request. 

Propagated HTTP Status Code from MultiResolutionImageResourceFetcher all
the way up to FaviconTabHelper, which required extension of 3 interfaces. 

BUG=39402
TEST=FaviconHandlerTest.UnableToDownloadFavicon

Reviewers:
sky@chromium.org - Overall CL, chrome/browser/*, content/browser/*
palmer@chromium.org - content/common/image_messages.h
joi@chromium.org - content/public/browser/web_contents.h
jamesr@chromium.org - content/renderer/*, webkit/glue/*
mnaganov@chromium.org - android_webview/browser/icon_helper.*

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

[mef, 2013-04-29 21:55:03]

Paul,

could you take a look? 

I've crafted a rough fix for favicon bug assigned to me, and it appears to work,
but I have a few questions.

thanks,
-m

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:359:
missing_favicon_urls_.insert(base::Hash(icon_url.spec()));
Should I somehow protect this set from multi-threaded access?

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:232: base::hash_set<int32> 
missing_favicon_urls_;
Lint suggests to add #include for hash_set<>, which appears to be hash_tables.h,
yet it is still unhappy.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_tab_helper.cc:141: VLOG(1) << "Skip Missing
FavIcon: " << url;
Should I somehow post a call to DidDownloadFavicon from here?

[pauljensen, 2013-04-30 14:38:04]

How did you come to conclude this was the right solution?  I scanned through the
bug report but didn't see much discussion on what the right solution was.  Have
you investigated the behavior of other browsers?  Are they following the HTTP
spec or are they performing additional caching of certain responses (404 etc)?

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:359:
missing_favicon_urls_.insert(base::Hash(icon_url.spec()));
On 2013/04/29 21:55:03, mef wrote:
> Should I somehow protect this set from multi-threaded access?
I would always tend to say yes, but I don't see any thread checks in this file. 
If all calls are on the same thread and it looks like this will always be the
case, then maybe not.  I'm not knowledgeable enough when it comes to
FaviconService to give a good answer, an OWNER of this file would know best.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:227: // Avoid repeated requests to
download missing favicon (crbug 39402).
The prevailing style is crbug.com/39402.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:232: base::hash_set<int32> 
missing_favicon_urls_;
On 2013/04/29 21:55:03, mef wrote:
> Lint suggests to add #include for hash_set<>, which appears to be
hash_tables.h,
> yet it is still unhappy.
Ya, I think lint is wrong and your code is correct.  I've run into this before.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:232: base::hash_set<int32> 
missing_favicon_urls_;
You should not use int32.  The Google C++ coding style recommends the C99 types
like int32_t, though I see base::Hash returns an uint32.  The Chromium coding
style says to use unsigned types for hash values.  I would recommend typedef'ing
this because you use it in two different files.  Maybe:
  typedef uint32 MissingFaviconURLHashValue;
or maybe with a shorter name.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_tab_helper.cc:141: VLOG(1) << "Skip Missing
FavIcon: " << url;
On 2013/04/29 21:55:03, mef wrote:
> Should I somehow post a call to DidDownloadFavicon from here?
I have no idea; I am very unfamiliar with the Favicon logic.  I'd recommend
finding a good OWNER.

[cbentzel, 2013-04-30 14:48:12]

I took an initial look at other browsers and noticed that favicon
requests were not following caching headers when they were not
present. I did not take a detailed look into what their behavior is -
for example, if caching headers exist do they obey it? I agree that we
should understand this behavior in more detail before beginning
implementation.

On Tue, Apr 30, 2013 at 10:38 AM,  <pauljensen@chromium.org> wrote:
> How did you come to conclude this was the right solution?  I scanned through
> the
> bug report but didn't see much discussion on what the right solution was.
> Have
> you investigated the behavior of other browsers?  Are they following the
> HTTP
> spec or are they performing additional caching of certain responses (404
> etc)?
>
>
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> File chrome/browser/favicon/favicon_service.cc (right):
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.cc:359:
> missing_favicon_urls_.insert(base::Hash(icon_url.spec()));
> On 2013/04/29 21:55:03, mef wrote:
>>
>> Should I somehow protect this set from multi-threaded access?
>
> I would always tend to say yes, but I don't see any thread checks in
> this file.  If all calls are on the same thread and it looks like this
> will always be the case, then maybe not.  I'm not knowledgeable enough
> when it comes to FaviconService to give a good answer, an OWNER of this
> file would know best.
>
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> File chrome/browser/favicon/favicon_service.h (right):
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.h:227: // Avoid repeated requests
>
> to download missing favicon (crbug 39402).
> The prevailing style is crbug.com/39402.
>
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.h:232: base::hash_set<int32>
> missing_favicon_urls_;
> On 2013/04/29 21:55:03, mef wrote:
>>
>> Lint suggests to add #include for hash_set<>, which appears to be
>
> hash_tables.h,
>>
>> yet it is still unhappy.
>
> Ya, I think lint is wrong and your code is correct.  I've run into this
> before.
>
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_service.h:232: base::hash_set<int32>
> missing_favicon_urls_;
> You should not use int32.  The Google C++ coding style recommends the
> C99 types like int32_t, though I see base::Hash returns an uint32.  The
> Chromium coding style says to use unsigned types for hash values.  I
> would recommend typedef'ing this because you use it in two different
> files.  Maybe:
>   typedef uint32 MissingFaviconURLHashValue;
> or maybe with a shorter name.
>
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> File chrome/browser/favicon/favicon_tab_helper.cc (right):
>
>
https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
> chrome/browser/favicon/favicon_tab_helper.cc:141: VLOG(1) << "Skip
> Missing FavIcon: " << url;
> On 2013/04/29 21:55:03, mef wrote:
>>
>> Should I somehow post a call to DidDownloadFavicon from here?
>
> I have no idea; I am very unfamiliar with the Favicon logic.  I'd
> recommend finding a good OWNER.
>
> https://codereview.chromium.org/14322023/

[mef, 2013-04-30 21:57:03]

Hi Steven,

I've recently joined the Chrome team and I'm working on the fix for
crbug.com/39402. 

From reading the discussion and my experiments Chrome is doing  the right thing
by requesting favicon on every request to  pages that don't have favicons, yet
Firefox remembers not to request missing favicon until browser is restarted. The
IE10 also remembers that icon is missing and re-requests it only after clearing
the browsing history.

I've crafted a potential fix by keeping track of missing favicons in hash_set
member on FaviconService. This achieves behavior similar to Firefox, where icon
is requested at most once during the session.

Does this make sense? 
Could you please review my changes? 
Do I need to worry about protection of the hash_set from concurrent access, or
favicon always gets accessed from one thread?
Is there anything else I should be aware of?

thanks,
-m

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.cc:359:
missing_favicon_urls_.insert(base::Hash(icon_url.spec()));
Sounds good, will do.
On 2013/04/30 14:38:04, pauljensen wrote:
> On 2013/04/29 21:55:03, mef wrote:
> > Should I somehow protect this set from multi-threaded access?
> I would always tend to say yes, but I don't see any thread checks in this
file. 
> If all calls are on the same thread and it looks like this will always be the
> case, then maybe not.  I'm not knowledgeable enough when it comes to
> FaviconService to give a good answer, an OWNER of this file would know best.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:227: // Avoid repeated requests to
download missing favicon (crbug 39402).
On 2013/04/30 14:38:04, pauljensen wrote:
> The prevailing style is crbug.com/39402.

Done.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:232: base::hash_set<int32> 
missing_favicon_urls_;
On 2013/04/30 14:38:04, pauljensen wrote:
> On 2013/04/29 21:55:03, mef wrote:
> > Lint suggests to add #include for hash_set<>, which appears to be
> hash_tables.h,
> > yet it is still unhappy.
> Ya, I think lint is wrong and your code is correct.  I've run into this
before.

Done.

https://codereview.chromium.org/14322023/diff/3001/chrome/browser/favicon/fav...
chrome/browser/favicon/favicon_service.h:232: base::hash_set<int32> 
missing_favicon_urls_;
On 2013/04/30 14:38:04, pauljensen wrote:
> On 2013/04/29 21:55:03, mef wrote:
> > Lint suggests to add #include for hash_set<>, which appears to be
> hash_tables.h,
> > yet it is still unhappy.
> Ya, I think lint is wrong and your code is correct.  I've run into this
before.

Done.

[stevenjb, 2013-04-30 22:30:38]

It is not entirely clear to me that this is desired behavior; I suspect that
many users run their browser for days and weeks even, during which time a
missing favicon might have been replaced. Perhaps a timeout should be used
instead?
+sky@ who should have more perspective on this.

[sky, 2013-05-01 00:39:57]

I agree with Thakis' comment 3, this should be a WontFix. Maybe I'm
missing something?

On Tue, Apr 30, 2013 at 3:30 PM,  <stevenjb@chromium.org> wrote:
> It is not entirely clear to me that this is desired behavior; I suspect that
> many users run their browser for days and weeks even, during which time a
> missing favicon might have been replaced. Perhaps a timeout should be used
> instead?
> +sky@ who should have more perspective on this.
>
>
> https://codereview.chromium.org/14322023/

[cbentzel, 2013-05-01 00:48:22]

See my comments 24-26 on the bug.

The primary goal is to stay compatible with other browsers. We should look
in more detail at what they do for 404 favicons.
On Apr 30, 2013 8:39 PM, "Scott Violet" <sky@chromium.org> wrote:

> I agree with Thakis' comment 3, this should be a WontFix. Maybe I'm
> missing something?
>
> On Tue, Apr 30, 2013 at 3:30 PM,  <stevenjb@chromium.org> wrote:
> > It is not entirely clear to me that this is desired behavior; I suspect
> that
> > many users run their browser for days and weeks even, during which time a
> > missing favicon might have been replaced. Perhaps a timeout should be
> used
> > instead?
> > +sky@ who should have more perspective on this.
> >
> >
> > https://codereview.chromium.org/14322023/
>

[sky, 2013-05-01 13:57:23]

Ah, ok. I didn't make it that far down the bug. I'll take a look shortly.

  -Scott

On Tue, Apr 30, 2013 at 5:48 PM, Chris Bentzel <cbentzel@chromium.org> wrote:
> See my comments 24-26 on the bug.
>
> The primary goal is to stay compatible with other browsers. We should look
> in more detail at what they do for 404 favicons.
>
> On Apr 30, 2013 8:39 PM, "Scott Violet" <sky@chromium.org> wrote:
>>
>> I agree with Thakis' comment 3, this should be a WontFix. Maybe I'm
>> missing something?
>>
>> On Tue, Apr 30, 2013 at 3:30 PM,  <stevenjb@chromium.org> wrote:
>> > It is not entirely clear to me that this is desired behavior; I suspect
>> > that
>> > many users run their browser for days and weeks even, during which time
>> > a
>> > missing favicon might have been replaced. Perhaps a timeout should be
>> > used
>> > instead?
>> > +sky@ who should have more perspective on this.
>> >
>> >
>> > https://codereview.chromium.org/14322023/

[sky, 2013-05-01 14:00:07]

https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:193: if (bitmaps.size() == 0) {
Under what circumstances should we be doing this? The comments indicate 404, yet
I don't think that empty bitmaps necessary means 404.

[mef, 2013-05-01 15:20:15]

Scott,

I've tried with Firefox 20 and IE 10 and they both don't request missing
favicon.ico on every page request. Firefox seems to flag missing icon in memory,
so after restart it requests it again. 

IE seems to store missing icon flag in disk cache, so it survives restart, and
re-requests missing favicon only clearing browser cache.

https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:193: if (bitmaps.size() == 0) {
My (possibly incorrect) understanding is that bitmaps vector will be empty if
request have failed for any reason or received data is not a valid image. As far
as I understand to handle 404 specifically this check needs to be done lower,
somewhere inside DownloadImage logic. I've put it here because I didn't want to
clutter DownloadImage with favicon specifics.

On 2013/05/01 14:00:07, sky wrote:
> Under what circumstances should we be doing this? The comments indicate 404,
yet
> I don't think that empty bitmaps necessary means 404.

[sky, 2013-05-01 16:17:51]

Chris's comments indicated we should only do this for a 404. Is it not
possible to provide that information to the download code? My worry is
it you went off line, tried to load a page, then went back online
presumably we wouldn't try to download the icon again until a restart.
I would assume similar badness would happen with other error
conditions.

  -Scott

On Wed, May 1, 2013 at 8:20 AM,  <mef@chromium.org> wrote:
> Scott,
>
> I've tried with Firefox 20 and IE 10 and they both don't request missing
> favicon.ico on every page request. Firefox seems to flag missing icon in
> memory,
> so after restart it requests it again.
>
> IE seems to store missing icon flag in disk cache, so it survives restart,
> and
> re-requests missing favicon only clearing browser cache.
>
>
>
>
>
>
https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
> File chrome/browser/favicon/favicon_tab_helper.cc (right):
>
>
https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
> chrome/browser/favicon/favicon_tab_helper.cc:193: if (bitmaps.size() ==
> 0) {
> My (possibly incorrect) understanding is that bitmaps vector will be
> empty if request have failed for any reason or received data is not a
> valid image. As far as I understand to handle 404 specifically this
> check needs to be done lower, somewhere inside DownloadImage logic. I've
> put it here because I didn't want to clutter DownloadImage with favicon
> specifics.
>
>
> On 2013/05/01 14:00:07, sky wrote:
>>
>> Under what circumstances should we be doing this? The comments
>
> indicate 404, yet
>>
>> I don't think that empty bitmaps necessary means 404.
>
>
> https://codereview.chromium.org/14322023/

[mef, 2013-05-01 16:24:15]

FWIW I've looked at Firefox source and their nsIFaviconService class keeps
in-memory cache (mFailedFavicons) of up to 256 'Failed' Favicons. There is no
expiration timeout, but they remove oldest entries if it grows over
MAX_FAVICON_CACHE_SIZE.

There is a method removeFailedFavicon and aForceReload flag for
SetAndLoadFaviconForPage method, but they don't appear to be used anywhere.

On 2013/04/30 22:30:38, stevenjb (chromium) wrote:
> It is not entirely clear to me that this is desired behavior; I suspect that
> many users run their browser for days and weeks even, during which time a
> missing favicon might have been replaced. Perhaps a timeout should be used
> instead?
> +sky@ who should have more perspective on this.

[mef, 2013-05-01 16:52:52]

The highest-level method that is aware of http response code is 
MultiResolutionImageResourceFetcher::OnURLFetchComplete, but unfortunately it
doesn't propagate it higher callbacks, and it doesn't know whether or not given
resource is a favicon.

I could propagate the response code back through
ImageLoadingHelper::DidDownloadImage => ImageHostMsg_DidDownloadImage message =>
 WebContentsImpl::OnDidDownloadImage => WebConents::ImageDownloadCallback =>
FaviconTabHelper::DidDownloadFavicon, but should I?

FWIW, Firefox doesn't distinguish between failure scenarios either - if Favicon
is retrieved or not valid, it goes onto failed list.

thanks,
-m

On 2013/05/01 16:17:51, sky wrote:
> Chris's comments indicated we should only do this for a 404. Is it not
> possible to provide that information to the download code? My worry is
> it you went off line, tried to load a page, then went back online
> presumably we wouldn't try to download the icon again until a restart.
> I would assume similar badness would happen with other error
> conditions.
> 
>   -Scott
> 
> On Wed, May 1, 2013 at 8:20 AM,  <mailto:mef@chromium.org> wrote:
> > Scott,
> >
> > I've tried with Firefox 20 and IE 10 and they both don't request missing
> > favicon.ico on every page request. Firefox seems to flag missing icon in
> > memory,
> > so after restart it requests it again.
> >
> > IE seems to store missing icon flag in disk cache, so it survives restart,
> > and
> > re-requests missing favicon only clearing browser cache.
> >
> >
> >
> >
> >
> >
>
https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
> > File chrome/browser/favicon/favicon_tab_helper.cc (right):
> >
> >
>
https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
> > chrome/browser/favicon/favicon_tab_helper.cc:193: if (bitmaps.size() ==
> > 0) {
> > My (possibly incorrect) understanding is that bitmaps vector will be
> > empty if request have failed for any reason or received data is not a
> > valid image. As far as I understand to handle 404 specifically this
> > check needs to be done lower, somewhere inside DownloadImage logic. I've
> > put it here because I didn't want to clutter DownloadImage with favicon
> > specifics.
> >
> >
> > On 2013/05/01 14:00:07, sky wrote:
> >>
> >> Under what circumstances should we be doing this? The comments
> >
> > indicate 404, yet
> >>
> >> I don't think that empty bitmaps necessary means 404.
> >
> >
> > https://codereview.chromium.org/14322023/

[sky, 2013-05-01 20:51:43]

IMO it's worth only caching if get 404. Additionally a shift-reload,
or whatever, should trigger loading always.

On Wed, May 1, 2013 at 9:52 AM,  <mef@chromium.org> wrote:
> The highest-level method that is aware of http response code is
> MultiResolutionImageResourceFetcher::OnURLFetchComplete, but unfortunately
> it
> doesn't propagate it higher callbacks, and it doesn't know whether or not
> given
> resource is a favicon.
>
> I could propagate the response code back through
> ImageLoadingHelper::DidDownloadImage => ImageHostMsg_DidDownloadImage
> message =>
>  WebContentsImpl::OnDidDownloadImage => WebConents::ImageDownloadCallback =>
> FaviconTabHelper::DidDownloadFavicon, but should I?
>
> FWIW, Firefox doesn't distinguish between failure scenarios either - if
> Favicon
> is retrieved or not valid, it goes onto failed list.
>
> thanks,
> -m
>
>
> On 2013/05/01 16:17:51, sky wrote:
>>
>> Chris's comments indicated we should only do this for a 404. Is it not
>> possible to provide that information to the download code? My worry is
>> it you went off line, tried to load a page, then went back online
>> presumably we wouldn't try to download the icon again until a restart.
>> I would assume similar badness would happen with other error
>> conditions.
>
>
>>    -Scott
>
>
>> On Wed, May 1, 2013 at 8:20 AM,  <mailto:mef@chromium.org> wrote:
>> > Scott,
>> >
>> > I've tried with Firefox 20 and IE 10 and they both don't request missing
>> > favicon.ico on every page request. Firefox seems to flag missing icon in
>> > memory,
>> > so after restart it requests it again.
>> >
>> > IE seems to store missing icon flag in disk cache, so it survives
>> > restart,
>> > and
>> > re-requests missing favicon only clearing browser cache.
>> >
>> >
>> >
>> >
>> >
>> >
>
>
>
https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
>>
>> > File chrome/browser/favicon/favicon_tab_helper.cc (right):
>> >
>> >
>
>
>
https://codereview.chromium.org/14322023/diff/11001/chrome/browser/favicon/fa...
>>
>> > chrome/browser/favicon/favicon_tab_helper.cc:193: if (bitmaps.size() ==
>> > 0) {
>> > My (possibly incorrect) understanding is that bitmaps vector will be
>> > empty if request have failed for any reason or received data is not a
>> > valid image. As far as I understand to handle 404 specifically this
>> > check needs to be done lower, somewhere inside DownloadImage logic. I've
>> > put it here because I didn't want to clutter DownloadImage with favicon
>> > specifics.
>> >
>> >
>> > On 2013/05/01 14:00:07, sky wrote:
>> >>
>> >> Under what circumstances should we be doing this? The comments
>> >
>> > indicate 404, yet
>> >>
>> >> I don't think that empty bitmaps necessary means 404.
>> >
>> >
>> > https://codereview.chromium.org/14322023/
>
>
>
> https://codereview.chromium.org/14322023/

[mef, 2013-05-03 17:51:49]

Hi Scott,

PTAL. 

I've propagated HTTP Status Code from MultiResolutionImageResourceFetcher all
the way up to FaviconTabHelper. Unfortunately it had added 10 files to the CL.
I've considered reusing the |requested_size| callback parameter in case of
failure, but that seemed as unclear hack, albeit one that would avoid an
avalanche of interface changes. What do you think?

I've also added a ClearMissingFavicons method that is used on 'Shift-Reload'
(RELOAD_IGNORING_CACHE), so user can always force re-request of the favicon.

thanks,
-m

[sky, 2013-05-03 20:24:53]

You're going to need more owners too (for webkit and content/public).

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:358: void
FaviconService::AddMissingFavicon(const GURL& icon_url) {
Order should match header.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:359: MissingFaviconURLHash url_hash =
base::Hash(icon_url.spec());
How likely are we to get false collisions?

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:371: 
nit: nuke newlines.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.h:227: // Avoid repeated requests to
download missing favicon (crbug.com/39402).
Nuke crbug reference.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.h:228: void AddMissingFavicon(const GURL&
icon_url);
The names here are mildly confusing. How about:
UnableToDownloadFavicon();
WasUnableToDownloadingFavicon();
ClearUnableToDownloadURLs();

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:141: VLOG(1) << "Skip Missing
FavIcon: " << url;
DVLOG ?

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:197: if (bitmaps.size() == 0 &&
http_status_code == 404) {
.empty()

https://codereview.chromium.org/14322023/diff/28001/content/renderer/image_lo...
File content/renderer/image_loading_helper.cc (right):

https://codereview.chromium.org/14322023/diff/28001/content/renderer/image_lo...
content/renderer/image_loading_helper.cc:57: 0,
Is 0 right here?

https://codereview.chromium.org/14322023/diff/28001/webkit/glue/multi_resolut...
File webkit/glue/multi_resolution_image_resource_fetcher.h (right):

https://codereview.chromium.org/14322023/diff/28001/webkit/glue/multi_resolut...
webkit/glue/multi_resolution_image_resource_fetcher.h:42: int http_status_code()
{ return http_status_code_; }
const?

[mef, 2013-05-03 21:45:59]

Hi Scott,

thanks a lot for your comments! PTAL, I've addressed them.

Should I reach out to webkit and content/public owners now or wait for your
approval?

thanks,
-m

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:358: void
FaviconService::AddMissingFavicon(const GURL& icon_url) {
On 2013/05/03 20:24:54, sky wrote:
> Order should match header.

Done.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:359: MissingFaviconURLHash url_hash =
base::Hash(icon_url.spec());
With ideal Hash it should be 1/2**32 ~ 25*(10**-11). Not very likely, but
theoretically possible. I was going for minimization of memory and performance
impact.

On 2013/05/03 20:24:54, sky wrote:
> How likely are we to get false collisions?

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:371: 
On 2013/05/03 20:24:54, sky wrote:
> nit: nuke newlines.

Done.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_service.h (right):

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.h:227: // Avoid repeated requests to
download missing favicon (crbug.com/39402).
On 2013/05/03 20:24:54, sky wrote:
> Nuke crbug reference.

Done.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.h:228: void AddMissingFavicon(const GURL&
icon_url);
On 2013/05/03 20:24:54, sky wrote:
> The names here are mildly confusing. How about:
> UnableToDownloadFavicon();
> WasUnableToDownloadingFavicon();
> ClearUnableToDownloadURLs();

Done.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:141: VLOG(1) << "Skip Missing
FavIcon: " << url;
On 2013/05/03 20:24:54, sky wrote:
> DVLOG ?

Done.

https://codereview.chromium.org/14322023/diff/28001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:197: if (bitmaps.size() == 0 &&
http_status_code == 404) {
On 2013/05/03 20:24:54, sky wrote:
> .empty()

Done.

https://codereview.chromium.org/14322023/diff/28001/content/renderer/image_lo...
File content/renderer/image_loading_helper.cc (right):

https://codereview.chromium.org/14322023/diff/28001/content/renderer/image_lo...
content/renderer/image_loading_helper.cc:57: 0,
On 2013/05/03 20:24:54, sky wrote:
> Is 0 right here?
I think it is more correct than 200, as no request was actually issued.

https://codereview.chromium.org/14322023/diff/28001/webkit/glue/multi_resolut...
File webkit/glue/multi_resolution_image_resource_fetcher.h (right):

https://codereview.chromium.org/14322023/diff/28001/webkit/glue/multi_resolut...
webkit/glue/multi_resolution_image_resource_fetcher.h:42: int http_status_code()
{ return http_status_code_; }
On 2013/05/03 20:24:54, sky wrote:
> const?

Done.

[sky, 2013-05-03 22:49:16]

I think this looks fine, can you add test coverage?

[mef, 2013-05-08 20:20:01]

Hi Scott,

thanks again for your reviews! I've added a TEST_F(FaviconHandlerTest,
UnableToDownloadFavicon). I didn't find any tests for FaviconTabHelper or
FaviconService, so I've piggy-backed on FaviconHandlerTest. Does this make
sense, or should I introduce a separate FaviconTabHelperTest instead?

thanks,
-m

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.h (right):

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.h:76: void DidDownloadFavicon(
I've made it public so I could access it from unit test. I'm not very happy
about this and would love suggestions on avoiding this.

[sky, 2013-05-09 14:49:47]

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_handler_unittest.cc:1029:
TEST_F(FaviconHandlerTest, UnableToDownloadFavicon) {
Add a description of what this is meant to test.

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_handler_unittest.cc:1036:
FaviconServiceFactory::GetInstance()->SetTestingFactory(
Does this need to be unset after your test (or TearDown)?

[mef, 2013-05-09 16:24:24]

Hi Scott,

PTAL. I've also added OWNERS to review their respective areas.

thanks,
-m

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_handler_unittest.cc (right):

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_handler_unittest.cc:1029:
TEST_F(FaviconHandlerTest, UnableToDownloadFavicon) {
On 2013/05/09 14:49:47, sky wrote:
> Add a description of what this is meant to test.

Done.

https://codereview.chromium.org/14322023/diff/60001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_handler_unittest.cc:1036:
FaviconServiceFactory::GetInstance()->SetTestingFactory(
On 2013/05/09 14:49:47, sky wrote:
> Does this need to be unset after your test (or TearDown)?

Done.

[sky, 2013-05-09 16:25:09]

LGTM

[Jói, 2013-05-09 17:06:42]

LGTM for content/public.

[jamesr, 2013-05-10 04:57:58]

webkit/ and content/renderer/ lgtm

[palmer, 2013-05-13 17:22:46]

IPC security LGTM.

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:264: MissingFaviconURLHash url_hash =
base::Hash(icon_url.spec());
I don't think it matters a whole lot, but my instinct is to use a std::set.

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:197: if (bitmaps.empty() &&
http_status_code == 404) {
Maybe not just 404, but any 400 error code? I don't know if that is best; just
an idea.

[mef, 2013-05-14 18:47:08]

Hi,

sorry for the spam. 

I've fixed errors on android and cros and that forced me to touch
android_webview/browser/icon_helper.*, so now I need an approval from one of
android_webview OWNERS.

thanks,
-m

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_service.cc (right):

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_service.cc:264: MissingFaviconURLHash url_hash =
base::Hash(icon_url.spec());
> I don't think it matters a whole lot, but my instinct is to use a std::set.
I thought about it, but hash_set seemed more natural fit as we don't care about
sorting order of missing url hashes.

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://codereview.chromium.org/14322023/diff/74001/chrome/browser/favicon/fa...
chrome/browser/favicon/favicon_tab_helper.cc:197: if (bitmaps.empty() &&
http_status_code == 404) {
On 2013/05/13 17:22:47, Chromium Palmer wrote:
> Maybe not just 404, but any 400 error code? I don't know if that is best; just
> an idea.
Initial CL did the same as Firefox - marked Favicon as missing on any error, but
per discussion with sky@ scope was limited to HTTP error 404, thus requiring
propagation of HTTP status up here.

[mnaganov (inactive), 2013-05-14 19:59:32]

android_webview LGTM

[commit-bot: I haz the power, 2013-05-14 20:29:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mef@chromium.org/14322023/99001

[commit-bot: I haz the power, 2013-05-15 08:37:50]

Change committed as 200194