Clear the favicon of a tab when navigating to a url on a different host.

Clear the favicon of a tab when navigating to a url on a different host.

Contributed by etienne@atnnn.com

BUG=28515

[pkotwicz, 2012-09-21 17:11:52]

I will take a look at the CL tomorrow. Thanks for looking into this!

[stevenjb, 2012-09-21 17:36:29]

msw@ - is this related to
http://code.google.com/p/chromium/issues/detail?id=128103

The change looks fine to me, but I haven't been in this code in a little while
now.

[msw, 2012-09-21 21:46:54]

Thanks for the patch! LGTM, but please get approval from someone more familiar
with the Favicon code; I worry this may flicker the default favicon before
retrieving the new domain's favicon (if so, this may need to happen later, in
cases where FaviconHandler::OnDidDownloadFavicon get no data or an error.

http://crbug.com/128103 is dup'ed to http://crbug.com/28515.
(this patch fixes that issue locally too)
I'll abandon my fix attempt at http://codereview.chromium.org/10968026, which
wasn't as good.

Note: I dislike assignment through the non-const ref
NavigationEntry::GetFavicon() accessor, but changing that is out of scope in
this CL.

https://chromiumcodereview.appspot.com/10886059/diff/1/chrome/browser/favicon...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

https://chromiumcodereview.appspot.com/10886059/diff/1/chrome/browser/favicon...
chrome/browser/favicon/favicon_tab_helper.cc:183: details.entry->GetFavicon() =
default_favicon;
nit: I think you can just do details.entry->GetFavicon() = FaviconStatus(); in
one line and drop the braces.

[pkotwicz, 2012-09-24 15:20:49]

As an update I am looking into redirects to see if this fix is in the correct
place. (I think it is).

[pkotwicz, 2012-09-28 20:01:23]

I am still looking at this, sorry that this is taking so long....

[pkotwicz, 2012-09-29 23:54:37]

LGTM with posted nit.

Adding sky@ for his insight.

http://codereview.chromium.org/10886059/diff/1/chrome/browser/favicon/favicon...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

http://codereview.chromium.org/10886059/diff/1/chrome/browser/favicon/favicon...
chrome/browser/favicon/favicon_tab_helper.cc:181: if
(details.previous_url.host() != details.entry->GetURL().host()) {
The favicon should actually always be cleared. (You can remove the if
statement).
I commented on the bug as to why I believe this should be the case. Sorry for
the delay

[AtnNn, 2012-09-30 06:47:19]

Thank you for the reviews.

> I worry this may flicker the default favicon before
retrieving the new domain's favicon

The a spinning "loading" icon hides the possible flicker.

In case it helps, I have updated the patch with your nits. Please commit if it
looks good.

[sky, 2012-10-01 14:33:53]

Can we have test coverage of this?

http://codereview.chromium.org/10886059/diff/9001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper.cc (right):

http://codereview.chromium.org/10886059/diff/9001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:180: // Reset the favicon
remove newline on 179.
This comment echos what the code does. It should describe 'why' the code is the
way it is.

[AtnNn, 2012-10-02 05:37:53]

On 2012/10/01 14:33:53, sky wrote:
> Can we have test coverage of this?

I can write a test this weekend.

[pkotwicz, 2012-10-24 16:52:48]

I added the tests in addition to your CL in
http://codereview.chromium.org/11198007/ which has landed last night.
Etienne, thanks for posting this CL and looking into this. This issue wouldn't
have caught my attention had you not posted a CL.