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
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163752

[pkotwicz, 2012-10-16 21:41:54]

Etienne thanks for the CL! I wrote a unittest and added a comment

Scott can you please take another look?

[sky, 2012-10-16 23:00:17]

LGTM

http://codereview.chromium.org/11198007/diff/6001/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_tab_helper_unittest.cc (right):

http://codereview.chromium.org/11198007/diff/6001/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper_unittest.cc:5: #include
"chrome/browser/favicon/favicon_tab_helper.h"
newline between 5/6.

[pkotwicz, 2012-10-17 01:29:34]

The code in this CL is based off of http://codereview.chromium.org/10886059/

[commit-bot: I haz the power, 2012-10-17 01:29:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/11198007/8

[pkotwicz, 2012-10-17 15:20:25]

Scott can you please take another look?

In this patch, I avoid clearing in the case of forward and back navigations just
to be safe.
If I clear the favicon in the case of forward and back navigations and I slow
down history enough, I can get favicons for sites to be removed from the back
menu by going back and forward really fast. Although I have not been able to
reproduce on slow devices such as the x86-alex Chromebook, I think it is good to
not clear in the case of forward and back navigations just to be safe

[sky, 2012-10-17 16:07:34]

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

http://codereview.chromium.org/11198007/diff/8002/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_tab_helper.cc:178: // The NavigationEntry may be
reused from the previously navigated page in
Is the real bug that we're reusing favicons on client redirect?

[pkotwicz, 2012-10-17 16:47:30]

Yes

[sky, 2012-10-17 21:15:19]

So, shouldn't we fix it in NavigationController rather than here?

  -Scott

On Wed, Oct 17, 2012 at 9:47 AM,  <pkotwicz@chromium.org> wrote:
> Yes
>
> http://codereview.chromium.org/11198007/

[pkotwicz, 2012-10-17 21:19:44]

Based on a previous email thread with creis@, "I don't think
NavigationControllerImpl knows anything about favicons, so it should probably
stay that way."

[pkotwicz, 2012-10-19 18:39:44]

Ping!

[sky, 2012-10-19 21:21:47]

I thought from the discussion the fix is in NavigationControllerImpl.
Maybe I'm missing something?

On Fri, Oct 19, 2012 at 11:39 AM,  <pkotwicz@chromium.org> wrote:
> Ping!
>
> http://codereview.chromium.org/11198007/

[sky, 2012-10-19 22:26:03]

Since FaviconStatus is in content shouldn't we move the fix there? This way
every content embedder doesn't need to deal with it.

[pkotwicz, 2012-10-21 19:49:13]

Scott and Charlie can you please take a look?

[sky, 2012-10-22 15:49:17]

LGTM

[Charlie Reis, 2012-10-23 04:53:22]

Hmm, this still seems strange to me that the NavigationController only knows
about favicons in a single edge case (redirects).  Where does the rest of the
favicon logic live?  Is any of it in the content module, or is it all in chrome?

I could be convinced otherwise, but at first glance this feels out of place.

https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl_unittest.cc:3011: //
Check that the favicon is not reused accross a client redirect.
nit: across

https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl_unittest.cc:3039:
favicon_status.valid = true;
Should probably test that DoImagesMatch is false here, to be sure that
DoImagesMatch is actually working.

https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
content/browser/web_contents/navigation_controller_impl_unittest.cc:3055: //
Check that the favicon is not cleared for NavigationEntrys which were
nit: NavigationEntries

[sky, 2012-10-23 15:55:16]

Here's my rationale for putting the logic here:
"Since FaviconStatus is in content shouldn't we move the fix there?
This way every content embedder doesn't need to deal with it."

  -Scott

On Mon, Oct 22, 2012 at 9:53 PM,  <creis@chromium.org> wrote:
> Hmm, this still seems strange to me that the NavigationController only knows
> about favicons in a single edge case (redirects).  Where does the rest of
> the
> favicon logic live?  Is any of it in the content module, or is it all in
> chrome?
>
> I could be convinced otherwise, but at first glance this feels out of place.
>
>
>
https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
> File content/browser/web_contents/navigation_controller_impl_unittest.cc
> (right):
>
>
https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:3011:
> // Check that the favicon is not reused accross a client redirect.
> nit: across
>
>
https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:3039:
> favicon_status.valid = true;
> Should probably test that DoImagesMatch is false here, to be sure that
> DoImagesMatch is actually working.
>
>
https://codereview.chromium.org/11198007/diff/15002/content/browser/web_conte...
> content/browser/web_contents/navigation_controller_impl_unittest.cc:3055:
> // Check that the favicon is not cleared for NavigationEntrys which were
> nit: NavigationEntries
>
> https://codereview.chromium.org/11198007/

[Charlie Reis, 2012-10-23 16:53:15]

Ok, but then it seems like most of chrome/browser/favicon/favicon_handler.cc
should move to content as well, right?  Right now other content embedders appear
to have no favicon updating logic.

I'm not opposed to that, but maybe we should file a bug to do that as cleanup. 
If that's in the plans (or if I'm misunderstanding how favicons are normally
updated), then this looks good.

On 2012/10/23 15:55:16, sky wrote:
> Here's my rationale for putting the logic here:
> "Since FaviconStatus is in content shouldn't we move the fix there?
> This way every content embedder doesn't need to deal with it."
> 
>   -Scott
> 
> On Mon, Oct 22, 2012 at 9:53 PM,  <mailto:creis@chromium.org> wrote:
> > Hmm, this still seems strange to me that the NavigationController only knows
> > about favicons in a single edge case (redirects).  Where does the rest of
> > the
> > favicon logic live?  Is any of it in the content module, or is it all in
> > chrome?
> >
> > I could be convinced otherwise, but at first glance this feels out of place.
> >

[Charlie Reis, 2012-10-23 17:58:29]

I just chatted with John (cc'd), and it sounds like filing a bug to move more of
the favicon updating into content is probably the right plan.  Please file that
and CC John, and then this LGTM.

Thanks!
Charlie


On 2012/10/23 16:53:15, creis wrote:
> Ok, but then it seems like most of chrome/browser/favicon/favicon_handler.cc
> should move to content as well, right?  Right now other content embedders
appear
> to have no favicon updating logic.
> 
> I'm not opposed to that, but maybe we should file a bug to do that as cleanup.

> If that's in the plans (or if I'm misunderstanding how favicons are normally
> updated), then this looks good.
> 
> On 2012/10/23 15:55:16, sky wrote:
> > Here's my rationale for putting the logic here:
> > "Since FaviconStatus is in content shouldn't we move the fix there?
> > This way every content embedder doesn't need to deal with it."
> > 
> >   -Scott
> > 
> > On Mon, Oct 22, 2012 at 9:53 PM,  <mailto:creis@chromium.org> wrote:
> > > Hmm, this still seems strange to me that the NavigationController only
knows
> > > about favicons in a single edge case (redirects).  Where does the rest of
> > > the
> > > favicon logic live?  Is any of it in the content module, or is it all in
> > > chrome?
> > >
> > > I could be convinced otherwise, but at first glance this feels out of
place.
> > >

[commit-bot: I haz the power, 2012-10-24 00:12:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pkotwicz@chromium.org/11198007/22003

[commit-bot: I haz the power, 2012-10-24 02:28:16]

Change committed as 163752