Expire favicons which were added with default favicon sizes

Expire favicons which were added with default favicon sizes

Bug=148669, 130896
Test=If a user clicks on a bookmark which is low res (it was added before hidpi favicons were added to Chrome, it becomes hidpi when the user clicks on it).
     The bookmark should be updated after the user has visited the page and the user restarted Chrome

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

[pkotwicz, 2012-09-12 16:22:31]

This CL forces Chrome to refetch any favicon data which was added before the
switch to hidpi favicons when the user visits the page.

Scott, can you please look at the patch as a whole?
Michael, can you comment on the OS_ANDROID if def?

[sky, 2012-09-12 17:04:08]

Won't this happen automatically as a result of db migration in previous
patches?

On Wed, Sep 12, 2012 at 9:22 AM,  <pkotwicz@chromium.org> wrote:
> Reviewers: sky, michaelbai,
>
> Message:
> This CL forces Chrome to refetch any favicon data which was added before the
> switch to hidpi favicons when the user visits the page.
>
> Scott, can you please look at the patch as a whole?
> Michael, can you comment on the OS_ANDROID if def?
>
>
>
> Description:
> Expire favicons which were added with default favicon sizes
>
> Bug=148669,130896
> Test=If a user clicks on a bookmark which is low res (it was added before
> hidpi
> favicons were added to Chrome, it becomes hidpi when the user clicks on it).
>      The bookmark should be updated after the user has visited the page and
> the
> user restarted Chrome
>
> Please review this at http://codereview.chromium.org/10918186/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M chrome/browser/extensions/extension_web_ui.cc
>   M chrome/browser/favicon/favicon_handler.h
>   M chrome/browser/favicon/favicon_handler.cc
>   M chrome/browser/favicon/favicon_handler_unittest.cc
>   M chrome/browser/ui/webui/chrome_web_ui_controller_factory.h
>   M chrome/browser/ui/webui/chrome_web_ui_controller_factory.cc
>
>

[michaelbai, 2012-09-12 17:05:18]

http://codereview.chromium.org/10918186/diff/3002/chrome/browser/favicon/favi...
File chrome/browser/favicon/favicon_handler.cc (right):

http://codereview.chromium.org/10918186/diff/3002/chrome/browser/favicon/favi...
chrome/browser/favicon/favicon_handler.cc:124: #if !defined(OS_ANDROID)
I don't think we should skip this logic entirely in Android. The icon added by
FaviconSQLHandler has icon_url empty.
The FaviconHandler should fetch the new icon if the icon url is not matched. It
is fine the icon was replaced by FaviconHandler for Android.

[pkotwicz, 2012-09-12 17:19:05]

The database migration does not set the last_updated time to 0, so I don't think
that this will happen automatically as a result. (All the favicon bitmaps do not
become expired).
We also need to handle the case where MergeFavicon() intentionally sets the
favicon sizes to empty.
Maybe I am missing something?

[sky, 2012-09-12 17:23:51]

Don't we end up treating an empty size string as needing to download all?

On Wed, Sep 12, 2012 at 10:19 AM,  <pkotwicz@chromium.org> wrote:
> The database migration does not set the last_updated time to 0, so I don't
> think
> that this will happen automatically as a result. (All the favicon bitmaps do
> not
> become expired).
> We also need to handle the case where MergeFavicon() intentionally sets the
> favicon sizes to empty.
> Maybe I am missing something?
>
> http://codereview.chromium.org/10918186/

[pkotwicz, 2012-09-12 17:26:18]

Yes, but FaviconHandler ignored the sizes attribute completely up till this CL.

[pkotwicz, 2012-09-12 17:31:46]

I think I may know what is causing the confusion.
HasExpiredOrIncompleteResult() is the incorrect place to check for the sizes
attribute in the long run. (Though I think it is an appropriate place for now).
It should be eventually checked in DoUrlsAndIconsMatch().
However, DoUrlsAndIconsMatch currently takes std::vector<FaviconBitmapResult>

[sky, 2012-09-12 21:27:51]

LGTM

[commit-bot: I haz the power, 2012-09-13 15:18:05]

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

[commit-bot: I haz the power, 2012-09-13 17:21:00]

Commit queue had an internal error.
Something went really wrong, probably a crash, a hickup or
simply the monkeys went out for dinner.
The relevant dude was pinged already.

[commit-bot: I haz the power, 2012-09-13 17:38:04]

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

[commit-bot: I haz the power, 2012-09-13 17:38:26]

Change committed as 156573