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

Review URL: https://codereview.chromium.org/1128343004

Cr-Commit-Position: refs/heads/master@{#330802}

chrome/browser/sync/test/integration/bookmarks_helper.cc

@@ -12,6 +12,7 @@
 #include "base/files/file_util.h"
 #include "base/path_service.h"
 #include "base/rand_util.h"
+#include "base/run_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
@@ -269,6 +270,25 @@ void SetFaviconImpl(Profile* profile,
     GetFaviconData(model, node);
 }
 
+// Expires the favicon for |profile| and |node|. |profile| may be
+// |test()->verifier()|.
+void ExpireFaviconImpl(Profile* profile, const BookmarkNode* node) {
+  favicon::FaviconService* favicon_service =
+      FaviconServiceFactory::GetForProfile(profile,
+                                           ServiceAccessType::EXPLICIT_ACCESS);
+  favicon_service->SetFaviconOutOfDateForPage(node->url());
+}
+
+// Called asynchronously from CheckFaviconExpired() with the favicon data from
+// the database.
+void OnGotFaviconForExpiryCheck(
+    const base::Closure& callback,
+    const favicon_base::FaviconRawBitmapResult& bitmap_result) {
+  ASSERT_TRUE(bitmap_result.is_valid());
+  ASSERT_TRUE(bitmap_result.expired);
+  callback.Run();
+}
+
 // Wait for all currently scheduled tasks on the history thread for all
 // profiles to complete and any notifications sent to the UI thread to have
 // finished processing.
@@ -585,6 +605,41 @@ void SetFavicon(int profile,
                  favicon_source);
 }
 
+void ExpireFavicon(int profile, const BookmarkNode* node) {
+  BookmarkModel* model = GetBookmarkModel(profile);
+  ASSERT_EQ(bookmarks::GetBookmarkNodeByID(model, node->id()), node)
+      << "Node " << node->GetTitle() << " does not belong to "
+      << "Profile " << profile;
+  ASSERT_EQ(BookmarkNode::URL, node->type()) << "Node " << node->GetTitle()
+                                             << " must be a url.";
+  ASSERT_EQ(1u, urls_with_favicons_->count(node->url()));
+
+  if (sync_datatype_helper::test()->use_verifier()) {
+    const BookmarkNode* v_node = nullptr;
+    FindNodeInVerifier(model, node, &v_node);
+    ExpireFaviconImpl(sync_datatype_helper::test()->verifier(), node);
+  }
+  ExpireFaviconImpl(sync_datatype_helper::test()->GetProfile(profile), node);
+
+  WaitForHistoryToProcessPendingTasks();
+}
+
+void CheckFaviconExpired(int profile, const GURL& icon_url) {
+  base::RunLoop run_loop;
+
+  favicon::FaviconService* favicon_service =
+      FaviconServiceFactory::GetForProfile(
+          sync_datatype_helper::test()->GetProfile(profile),
+          ServiceAccessType::EXPLICIT_ACCESS);
+  base::CancelableTaskTracker task_tracker;
+  favicon_service->GetRawFavicon(
+      icon_url, favicon_base::FAVICON, 0,
+      base::Bind(&OnGotFaviconForExpiryCheck, run_loop.QuitClosure()),
+      &task_tracker);
+
+  run_loop.Run();
+}
+
 const BookmarkNode* SetURL(int profile,
                            const BookmarkNode* node,
                            const GURL& new_url) {

chrome/browser/sync/test/integration/bookmarks_helper.h

@@ -106,6 +106,12 @@ void SetFavicon(int profile,
                 const gfx::Image& image,
                 FaviconSource source);
 
+// Expires the favicon for |node| in the bookmark model for |profile|.
+void ExpireFavicon(int profile, const bookmarks::BookmarkNode* node);
+
+// Checks whether the favicon at |icon_url| for |profile| is expired;
+void CheckFaviconExpired(int profile, const GURL& icon_url);
+
 // Changes the url of the node |node| in the bookmark model of profile
 // |profile| to |new_url|. Returns a pointer to the node with the changed url.
 const bookmarks::BookmarkNode* SetURL(int profile,

chrome/browser/sync/test/integration/two_client_bookmarks_sync_test.cc

@@ -25,9 +25,11 @@ using bookmarks_helper::AddFolder;
 using bookmarks_helper::AddURL;
 using bookmarks_helper::AllModelsMatch;
 using bookmarks_helper::AllModelsMatchVerifier;
+using bookmarks_helper::CheckFaviconExpired;
 using bookmarks_helper::ContainsDuplicateBookmarks;
 using bookmarks_helper::CountBookmarksWithTitlesMatching;
 using bookmarks_helper::CreateFavicon;
+using bookmarks_helper::ExpireFavicon;
 using bookmarks_helper::GetBookmarkBarNode;
 using bookmarks_helper::GetManagedNode;
 using bookmarks_helper::GetOtherNode;
@@ -229,6 +231,54 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_SetFaviconHiDPI) {
   ASSERT_TRUE(AllModelsMatchVerifier());
 }
 
+// Test that if sync does not modify a favicon bitmap's data that it does not
+// modify the favicon bitmap's "last updated time" either. This is important
+// because the last updated time is used to determine whether a bookmark's
+// favicon should be redownloaded when the web when the bookmark is visited.
+// If sync prevents the "last updated time" from expiring, the favicon is
+// never redownloaded from the web. (http://crbug.com/481414)
+IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
+                       SC_UpdatingTitleDoesNotUpdateFaviconLastUpdatedTime) {
+  ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
+
+  std::vector<ui::ScaleFactor> supported_scale_factors;
+  supported_scale_factors.push_back(ui::SCALE_FACTOR_100P);
+  ui::SetSupportedScaleFactors(supported_scale_factors);
+
+  const GURL page_url(kGenericURL);
+  const GURL icon_url("http://www.google.com/favicon.ico");
+
+  ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  const BookmarkNode* bookmark0 = AddURL(0, kGenericURLTitle, page_url);
+  ASSERT_NE(bookmark0, nullptr);
+  gfx::Image favicon0 = CreateFavicon(SK_ColorBLUE);
+  ASSERT_FALSE(favicon0.IsEmpty());
+  SetFavicon(0, bookmark0, icon_url, favicon0, bookmarks_helper::FROM_UI);
+
+  // Expire the favicon (e.g. as a result of the user "clearing browsing
+  // history from the beginning of time")
+  ExpireFavicon(0, bookmark0);
+  CheckFaviconExpired(0, icon_url);
+
+  ASSERT_TRUE(GetClient(0)->AwaitMutualSyncCycleCompletion(GetClient(1)));
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  // Change the bookmark's title for profile 1. Changing the title will cause
+  // the bookmark's favicon data to be synced from profile 1 to profile 0 even
+  // though the favicon data did not change.
+  const std::string kNewTitle = "New Title";
+  ASSERT_NE(kNewTitle, kGenericURLTitle);
+  const BookmarkNode* bookmark1 = GetUniqueNodeByURL(1, page_url);
+  SetTitle(1, bookmark1, kNewTitle);
+  ASSERT_TRUE(GetClient(1)->AwaitMutualSyncCycleCompletion(GetClient(0)));
+  ASSERT_TRUE(AllModelsMatchVerifier());
+
+  // The favicon for profile 0 should still be expired.
+  CheckFaviconExpired(0, icon_url);
+}
+
 // Test Scribe ID - 370560.
 IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_AddNonHTTPBMs) {
   ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";

components/history/core/browser/history_backend.cc

@@ -1603,8 +1603,12 @@ void HistoryBackend::MergeFavicon(
   for (size_t i = 0; i < bitmap_id_sizes.size(); ++i) {
     if (bitmap_id_sizes[i].pixel_size == pixel_size) {
       if (IsFaviconBitmapDataEqual(bitmap_id_sizes[i].bitmap_id, bitmap_data)) {
-        thumbnail_db_->SetFaviconBitmapLastUpdateTime(
-            bitmap_id_sizes[i].bitmap_id, base::Time::Now());
+        // Sync calls MergeFavicon() for all of the favicons that it manages at
+        // startup. Do not update the "last updated" time if the favicon bitmap
+        // data matches that in the database.
+        // TODO: Pass in boolean to MergeFavicon() if any users of
+        // MergeFavicon() want the last_updated time to be updated when the new
+        // bitmap data is identical to the old.
         bitmap_identical = true;
       } else {
         // Expire the favicon bitmap because sync can provide incorrect

components/history/core/browser/history_backend.h

@@ -491,6 +491,8 @@ class HistoryBackend : public base::RefCountedThreadSafe<HistoryBackend>,
                            MergeFaviconIconURLMappedToDifferentPageURL);
   FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest,
                            MergeFaviconMaxFaviconBitmapsPerIconURL);
+  FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest,
+                           MergeIdenticalFaviconDoesNotChangeLastUpdatedTime);
   FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest,
                            UpdateFaviconMappingsAndFetchMultipleIconTypes);
   FRIEND_TEST_ALL_PREFIXES(HistoryBackendTest, GetFaviconsFromDBEmpty);

components/history/core/browser/history_backend_unittest.cc

@@ -2324,6 +2324,56 @@ TEST_F(HistoryBackendTest, MergeFaviconShowsUpInGetFaviconsForURLResult) {
   EXPECT_TRUE(BitmapDataEqual('c', result.bitmap_data));
 }
 
+// Tests that calling MergeFavicon() with identical favicon data does not affect
+// the favicon bitmap's "last updated" time. This is important because sync
+// calls MergeFavicon() for all of the favicons that it manages at startup.
+TEST_F(HistoryBackendTest, MergeIdenticalFaviconDoesNotChangeLastUpdatedTime) {
+  GURL page_url("http://www.google.com");
+  GURL icon_url("http://www.google.com/favicon.ico");
+  std::vector<unsigned char> data;
+  data.push_back('a');
+
+  scoped_refptr<base::RefCountedBytes> bitmap_data(
+      new base::RefCountedBytes(data));
+  backend_->MergeFavicon(page_url,
+                         icon_url,
+                         favicon_base::FAVICON,
+                         bitmap_data,
+                         kSmallSize);
+
+  // Find the ID of the add favicon bitmap.
+  std::vector<IconMapping> icon_mappings;
+  ASSERT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL(page_url,
+      &icon_mappings));
+  ASSERT_EQ(1u, icon_mappings.size());
+  std::vector<FaviconBitmap> favicon_bitmaps;
+  ASSERT_TRUE(backend_->thumbnail_db_->GetFaviconBitmaps(
+      icon_mappings[0].icon_id, &favicon_bitmaps));
+
+  // Change the last updated time of the just added favicon bitmap.
+  const base::Time kLastUpdateTime =
+      base::Time::Now() - base::TimeDelta::FromDays(314);
+  backend_->thumbnail_db_->SetFaviconBitmapLastUpdateTime(
+      favicon_bitmaps[0].bitmap_id, kLastUpdateTime);
+
+  // Call MergeFavicon() with identical data.
+  backend_->MergeFavicon(page_url,
+                         icon_url,
+                         favicon_base::FAVICON,
+                         bitmap_data,
+                         kSmallSize);
+
+  // Check that the "last updated" time did not change.
+  icon_mappings.clear();
+  ASSERT_TRUE(backend_->thumbnail_db_->GetIconMappingsForPageURL(page_url,
+      &icon_mappings));
+  ASSERT_EQ(1u, icon_mappings.size());
+  favicon_bitmaps.clear();
+  ASSERT_TRUE(backend_->thumbnail_db_->GetFaviconBitmaps(
+      icon_mappings[0].icon_id, &favicon_bitmaps));
+  EXPECT_EQ(kLastUpdateTime, favicon_bitmaps[0].last_updated);
+}
+
 // Tests GetFaviconsForURL with icon_types priority,
 TEST_F(HistoryBackendTest, TestGetFaviconsForURLWithIconTypesPriority) {
   GURL page_url("http://www.google.com");