Expire favicon bitmaps when merging from sync and clearing browser history

Expire favicon bitmaps when merging from sync and clearing browser history

As a result of crbug.com/474421, sync sometimes stores incorrect data to the
favicons database.

This CL:
- Expires bookmark favicon bitmaps when history is cleared
- Expires the favicon bitmap data if MergeFavicon() overwrites the current
  favicon bitmap. MergeFavicon is the entry point used by sync to update
  favicons. MergeFavicon() has other callers but they pass in the page URL as
  the icon URL.

BUG=474613
TEST=ThumbnailDatabaseTest.*, BrowsingDataRemoverTest.*

Committed: https://crrev.com/bf4296badb82df64393afcf168302c9bae736d6a
Cr-Commit-Position: refs/heads/master@{#326819}

[pkotwicz, 2015-04-21 00:27:18]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-04-21 00:45:54]

Patchset #1 (id:10007) has been deleted

[pkotwicz, 2015-04-21 00:47:41]

pkotwicz@chromium.org changed reviewers:
+ sky@chromium.org

[pkotwicz, 2015-04-21 00:47:42]

sky@ PTAL

[pkotwicz, 2015-04-21 00:47:43]

sky@ PTAL

[sky, 2015-04-21 13:20:42]

"Expires bookmark favicon bitmaps when history is cleared"

We explicitly decided we did not want to do this. Rationale is it would mean
bookmark bar and any other place that surfaces bookmarks has no favicon.

[pkotwicz, 2015-04-21 13:29:25]

sky@ can you please take another look?

I am expiring but NOT deleting the favicon for bookmarks. This has the side
effect of causing the favicon to be redownloaded when the user visits the
bookmarked site.

My understanding is that ExpireHistoryBackend does not delete favicons for
bookmarked pages.

The goal of this CL is to make it easier for a user to recover from sync
screwing their bookmark favicons up.

[pkotwicz, 2015-04-21 13:32:29]

Full disclosure, ExpireHistoryBackend can cause bookmarked favicons to be
deleted, but that bug is not made better or worse with this CL. It is logged at
http://crbug.com/478944

[sky, 2015-04-21 23:28:18]

I don't like working around bugs in other code.

On Tue, Apr 21, 2015 at 6:29 AM,  <pkotwicz@chromium.org> wrote:
> sky@ can you please take another look?
>
> I am expiring but NOT deleting the favicon for bookmarks. This has the side
> effect of causing the favicon to be redownloaded when the user visits the
> bookmarked site.
>
> My understanding is that ExpireHistoryBackend does not delete favicons for
> bookmarked pages.
>
> The goal of this CL is to make it easier for a user to recover from sync
> screwing their bookmark favicons up.
>
> https://codereview.chromium.org/1098053003/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pkotwicz, 2015-04-22 00:01:18]

The code in ThumbnailDatabase::RetainDataForPageUrls() is not working around a
bug. It is reasonable for a user to expect that clearing a sufficient amount of
browser data in chrome://history will allow them to get new bookmark favicons
when visiting a site. Forcing a user to wait 7 days is not very user friendly.
(I am open to discussing whether this should be done as part of clearing
"browsing history" or as part of something else.) For M44, my main goal is that
if a user clears absolutely all of their browser data (history, cached images,
download history, passwords etc) that it causes the bookmark favicons to get
redownloaded when a user visits the bookmark. I suspect that this is the 99% use
case. 

Yes, the change in HistoryBackend::MergeFavicon() is clearly working around the
sync server bug. Unfortunately, the sync server bug cannot be fixed by the end
of this milestone or the next so it is necessary to work around the bug. See
comment #13 in http://crbug.com/474421 for details.

According to comment #23 in crbug.com/454150 " This was the #1 report from M40
from our consumer feedback team and it really needs to be addressed." I want to
fix this bug as completely as possible. See the blocking bugs for
crbug.com/454150 for a full list of the favicon bugs that I am fixing for M44

The sync server bug that this CL works around is new. And yes, all of the other
bugs that I am fixing have been around for 2+ years.

[sky, 2015-04-23 15:34:49]

Fair enough. There is one use of MergeFavicons from partner_bookmarks_reader. I
can't tell if that wants the new expiring logic. Also, please update the
description for mergefavicons.

[pkotwicz, 2015-04-23 17:51:22]

I have updated the CL description for MergeFavicon()

partner_bookmarks_reader.cc cannot care about the expiry logic because it passes
the page URL as the icon URL. When a user visits the page, we will always
redownload the favicon because the icon URLs will not match.

[sky, 2015-04-23 21:50:53]

Ok, LGTM

[pkotwicz, 2015-04-24 15:38:21]

The CQ bit was checked by pkotwicz@chromium.org

[commit-bot: I haz the power, 2015-04-24 15:39:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1098053003/30001

[commit-bot: I haz the power, 2015-04-24 16:50:40]

Committed patchset #1 (id:30001)

[commit-bot: I haz the power, 2015-04-24 16:52:28]

Patchset 1 (id:??) landed as
https://crrev.com/bf4296badb82df64393afcf168302c9bae736d6a
Cr-Commit-Position: refs/heads/master@{#326819}