Allow favicons for synced bookmarks to expire

Allow favicons for synced bookmarks to expire

This CL prevents sync from updating the "last updated time" for bookmark
favicons each time that Chrome is started. The "last updated time" determines
when the favicon is redownloaded from the web (in case it has been updated).

Sync calls HistoryBackend::MergeFavicon() for each bookmark favicon on startup.
This CL stops HistoryBackend::MergeFavicon() from updating the "last updated
time" when MergeFavicon() is called with the same bitmap data as already stored
in the database (no new bitmap data).

BUG=481414
TEST=TwoClientBookmarksSyncTest.SC_UpdatingTitleDoesNotUpdateFaviconLastUpdatedTime

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

[pkotwicz, 2015-05-10 02:10:44]

pkotwicz@chromium.org changed reviewers:
+ michaelbai@chromium.org, zea@chromium.org

[pkotwicz, 2015-05-10 02:14:49]

zea@ can you please take a look at the new sync integration test?

michaelbai@ can you please take a look at how this affects
partner_bookmarks_reader.cc My understanding is that partner_bookmarks_reader.cc
calls MergeFavicon() for each partner bookmark at startup in which case the new
behavior is the desired behavior.

[pkotwicz, 2015-05-10 02:24:08]

Patchset #1 (id:1) has been deleted

[pkotwicz, 2015-05-10 02:33:32]

Patchset #2 (id:2) has been deleted

[pkotwicz, 2015-05-10 02:35:52]

Patchset #1 (id:20001) has been deleted

[pkotwicz, 2015-05-10 02:41:50]

Patchset #1 (id:50001) has been deleted

[pkotwicz, 2015-05-10 02:41:55]

Patchset #1 (id:50001) has been deleted

[pkotwicz, 2015-05-10 02:42:07]

Patchset #1 (id:50001) has been deleted

[pkotwicz, 2015-05-10 02:42:15]

Patchset #1 (id:20002) has been deleted

[Nicolas Zea, 2015-05-14 04:08:23]

Mostly LG, one question

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:272: void
ExpireFaviconImpl(Profile* profile, const BookmarkNode* node) {
nit: comment for these two methods and what they're for

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:252:
ASSERT_TRUE(bookmark0 != nullptr);
nit: ASSERT_NE?

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:257: //
Expire the favicon (e.g. as a result of the user "clearing browsing
Could you explain why we need to expire the favicon before doing the change?

[pkotwicz, 2015-05-14 14:53:42]

Patchset #2 (id:100001) has been deleted

[pkotwicz, 2015-05-14 14:54:36]

Nicolas, can you please take another look?

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/bookmarks_helper.cc:272: void
ExpireFaviconImpl(Profile* profile, const BookmarkNode* node) {
On 2015/05/14 04:08:23, Nicolas Zea wrote:
> nit: comment for these two methods and what they're for 

Done.

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
(right):

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:252:
ASSERT_TRUE(bookmark0 != nullptr);
On 2015/05/14 04:08:23, Nicolas Zea wrote:
> nit: ASSERT_NE?

Done.

https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:257: //
Expire the favicon (e.g. as a result of the user "clearing browsing
SetFavicon() on line 255 adds an unexpired favicon to the database. In fact,
FaviconService::SetFavicons() always sets the "last updated time" to the current
time.

"Expiring the favicon" emulates either:
The favicon getting out of date naturally (i.e. as a result of time passing)
The favicon getting expired as a result of the user "clearing history from the
beginning of time" via chrome://history

The test that I would love to write but am unsure how to is:
1) expire the favicon
2) emulate sync startup (i.e. restarting Chrome)
3) check that the favicon is still expired

[Nicolas Zea, 2015-05-14 18:11:31]

On 2015/05/14 14:54:36, pkotwicz wrote:
> Nicolas, can you please take another look?
> 
>
https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/bookmarks_helper.cc (right):
> 
>
https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/bookmarks_helper.cc:272: void
> ExpireFaviconImpl(Profile* profile, const BookmarkNode* node) {
> On 2015/05/14 04:08:23, Nicolas Zea wrote:
> > nit: comment for these two methods and what they're for 
> 
> Done.
> 
>
https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
> File chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc
> (right):
> 
>
https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:252:
> ASSERT_TRUE(bookmark0 != nullptr);
> On 2015/05/14 04:08:23, Nicolas Zea wrote:
> > nit: ASSERT_NE?
> 
> Done.
> 
>
https://codereview.chromium.org/1128343004/diff/80001/chrome/browser/sync/tes...
> chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc:257: //
> Expire the favicon (e.g. as a result of the user "clearing browsing
> SetFavicon() on line 255 adds an unexpired favicon to the database. In fact,
> FaviconService::SetFavicons() always sets the "last updated time" to the
current
> time.
> 
> "Expiring the favicon" emulates either:
> The favicon getting out of date naturally (i.e. as a result of time passing)
> The favicon getting expired as a result of the user "clearing history from the
> beginning of time" via chrome://history
> 
> The test that I would love to write but am unsure how to is:
> 1) expire the favicon
> 2) emulate sync startup (i.e. restarting Chrome)
> 3) check that the favicon is still expired

Does that mean that expiring the favicon won't actually delete the favicon
information? Just to double check, if the favicon gets deleted, and then a new
bookmark with that favicon is added, the favicon will still be brought back
right?

[pkotwicz, 2015-05-14 18:24:28]

We have special logic so that favicons which are associated with bookmarks are
never deleted.
You are right that non-bookmarked favicons are deleted when they expire

[Nicolas Zea, 2015-05-14 18:27:57]

Ah right, makes sense.

Sync changes LGTM, with one request: Could you update the comment for the test
to better explain why expiration is part of the test? As I understand it,
expiration is a proxy for checking last visited time, which is what's actually
not supposed to update, right?

[michaelbai, 2015-05-14 19:08:32]

michaelbai@chromium.org changed reviewers:
+ aruslan@chromium.org

[michaelbai, 2015-05-14 19:08:33]

lgtm, added aruslan@ for double check.

[aruslan, 2015-05-14 20:11:27]

lgtm

[pkotwicz, 2015-05-14 21:44:25]

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

[pkotwicz, 2015-05-14 21:44:26]

Scott, can you please take a look?

This CL is more important than https://codereview.chromium.org/1133463005/

[sky, 2015-05-18 17:15:20]

Only the history change, right?

LGTM, make sure to update any relavent docs. Also, I'm surprised we don't have
test coverage in the history of MergeFavicon.

https://codereview.chromium.org/1128343004/diff/140001/components/history/cor...
File components/history/core/browser/history_backend.cc (right):

https://codereview.chromium.org/1128343004/diff/140001/components/history/cor...
components/history/core/browser/history_backend.cc:1605: if
(IsFaviconBitmapDataEqual(bitmap_id_sizes[i].bitmap_id, bitmap_data)) {
How about a comment as to why this doesn't update the time?

[pkotwicz, 2015-05-20 20:00:12]

Patchset #4 (id:160001) has been deleted

[pkotwicz, 2015-05-20 20:01:11]

Patchset #4 (id:180001) has been deleted

[pkotwicz, 2015-05-20 20:04:03]

I have modified the comment and have added a test in history_backend_unittest.cc
I care much more about the sync test because from an API standpoint, it is
unclear what MergeFavicon() should do with the last updated time. The change to
MergeFavicon() is due to how sync uses MergeFavicon()

[pkotwicz, 2015-05-20 20:04:14]

The CQ bit was checked by pkotwicz@chromium.org

[pkotwicz, 2015-05-20 20:04:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaelbai@chromium.org, aruslan@chromium.org, zea@chromium.org,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1128343004/#ps200001
(title: " ")

[commit-bot: I haz the power, 2015-05-20 20:04:33]

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

[commit-bot: I haz the power, 2015-05-20 21:22:15]

Committed patchset #4 (id:200001)

[commit-bot: I haz the power, 2015-05-20 21:23:09]

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