Commit d63652c8 authored by Mikel Astiz's avatar Mikel Astiz Committed by Commit Bot

Extend bookmark sync code with support for favicon deletion

Recent patches have introduced new favicon scenarios by handling cases
where a site's content changes to not contain favicons.

Updating sync code to handle these events is needed to prevent sync
from resurrecting the favicon, due to not knowing about the deletion
and hence sync-ing it back to the local cache.

One notable change is that the new logic will introduce the concept of
sync-ed bookmark protos having neither an icon URL (field |icon_url|)
nor favicon image data (field |favicon|). Ancient versions of chrome
(prior to M25) didn't sync these fields and would be incompatible
(although not really badly) but too old for us to care.

Manually tested sync-ing across two patched instances of Chrome desktop
(Linux):
- Propagate deletions of bookmark favicons (after page content changes).
- Restoring deleted bookmark favicons (after page content changes).

Bug: 85806
Change-Id: Ia3534c6094983658f59f973777b5ed649c8769e8
Reviewed-on: https://chromium-review.googlesource.com/704737
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: 's avatarScott Violet <sky@chromium.org>
Reviewed-by: 's avatarNicolas Zea <zea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#514424}
parent 04ddb519
......@@ -90,7 +90,6 @@ class FaviconChangeObserver : public bookmarks::BookmarkModelObserver {
wait_for_load_ = true;
content::RunMessageLoop();
ASSERT_TRUE(node_->is_favicon_loaded());
ASSERT_FALSE(model_->GetFavicon(node_).IsEmpty());
}
void WaitForSetFavicon() {
wait_for_load_ = false;
......@@ -139,12 +138,6 @@ class FaviconChangeObserver : public bookmarks::BookmarkModelObserver {
DISALLOW_COPY_AND_ASSIGN(FaviconChangeObserver);
};
// A collection of URLs for which we have added favicons. Since loading a
// favicon is an asynchronous operation and doesn't necessarily invoke a
// callback, this collection is used to determine if we must wait for a URL's
// favicon to load or not.
std::set<GURL>* urls_with_favicons_ = nullptr;
// Returns the number of nodes of node type |node_type| in |model| whose
// titles match the string |title|.
int CountNodesWithTitlesMatching(BookmarkModel* model,
......@@ -222,14 +215,8 @@ struct FaviconData {
// Gets the favicon and icon URL associated with |node| in |model|.
FaviconData GetFaviconData(BookmarkModel* model,
const BookmarkNode* node) {
// If a favicon wasn't explicitly set for a particular URL, simply return its
// blank favicon.
if (!urls_with_favicons_ ||
urls_with_favicons_->find(node->url()) == urls_with_favicons_->end()) {
return FaviconData();
}
// If a favicon was explicitly set, we may need to wait for it to be loaded
// via BookmarkModel::GetFavicon(), which is an asynchronous operation.
// We may need to wait for the favicon to be loaded via
// BookmarkModel::GetFavicon(), which is an asynchronous operation.
if (!node->is_favicon_loaded()) {
FaviconChangeObserver observer(model, node);
model->GetFavicon(node);
......@@ -278,24 +265,50 @@ void ExpireFaviconImpl(Profile* profile, const BookmarkNode* node) {
favicon_service->SetFaviconOutOfDateForPage(node->url());
}
// Called asynchronously from CheckFaviconExpired() with the favicon data from
// the database.
void OnGotFaviconForExpiryCheck(
// Used to call FaviconService APIs synchronously by making |callback| quit a
// RunLoop.
void OnGotFaviconData(
const base::Closure& callback,
favicon_base::FaviconRawBitmapResult* output_bitmap_result,
const favicon_base::FaviconRawBitmapResult& bitmap_result) {
ASSERT_TRUE(bitmap_result.is_valid());
ASSERT_TRUE(bitmap_result.expired);
*output_bitmap_result = bitmap_result;
callback.Run();
}
// Deletes favicon mappings for |profile| and |node|. |profile| may be
// |test()->verifier()|.
void DeleteFaviconMappingsImpl(Profile* profile,
const BookmarkNode* node,
bookmarks_helper::FaviconSource favicon_source) {
BookmarkModel* model = BookmarkModelFactory::GetForBrowserContext(profile);
FaviconChangeObserver observer(model, node);
favicon::FaviconService* favicon_service =
FaviconServiceFactory::GetForProfile(profile,
ServiceAccessType::EXPLICIT_ACCESS);
if (favicon_source == bookmarks_helper::FROM_UI) {
favicon_service->DeleteFaviconMappings({node->url()},
favicon_base::IconType::kFavicon);
} else {
browser_sync::ProfileSyncService* pss =
ProfileSyncServiceFactory::GetForProfile(profile);
sync_bookmarks::BookmarkChangeProcessor::ApplyBookmarkFavicon(
node, pss->GetSyncClient(), /*icon_url=*/GURL(),
scoped_refptr<base::RefCountedString>(new base::RefCountedString()));
}
// Wait for the favicon for |node| to be invalidated.
observer.WaitForSetFavicon();
// Wait for the BookmarkModel to fetch the updated favicon and for the new
// favicon to be sent to BookmarkChangeProcessor.
GetFaviconData(model, node);
}
// 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.
void WaitForHistoryToProcessPendingTasks() {
// Skip waiting for history to complete for tests without favicons.
if (!urls_with_favicons_)
return;
std::vector<Profile*> profiles_which_need_to_wait;
if (sync_datatype_helper::test()->use_verifier())
profiles_which_need_to_wait.push_back(
......@@ -587,9 +600,6 @@ void SetFavicon(int profile,
<< "Profile " << profile;
ASSERT_EQ(BookmarkNode::URL, node->type()) << "Node " << node->GetTitle()
<< " must be a url.";
if (urls_with_favicons_ == nullptr)
urls_with_favicons_ = new std::set<GURL>();
urls_with_favicons_->insert(node->url());
if (sync_datatype_helper::test()->use_verifier()) {
const BookmarkNode* v_node = nullptr;
FindNodeInVerifier(model, node, &v_node);
......@@ -613,7 +623,6 @@ void ExpireFavicon(int profile, const BookmarkNode* node) {
<< "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;
......@@ -633,12 +642,55 @@ void CheckFaviconExpired(int profile, const GURL& icon_url) {
sync_datatype_helper::test()->GetProfile(profile),
ServiceAccessType::EXPLICIT_ACCESS);
base::CancelableTaskTracker task_tracker;
favicon_base::FaviconRawBitmapResult bitmap_result;
favicon_service->GetRawFavicon(
icon_url, favicon_base::IconType::kFavicon, 0,
base::Bind(&OnGotFaviconForExpiryCheck, run_loop.QuitClosure()),
base::Bind(&OnGotFaviconData, run_loop.QuitClosure(), &bitmap_result),
&task_tracker);
run_loop.Run();
ASSERT_TRUE(bitmap_result.is_valid());
ASSERT_TRUE(bitmap_result.expired);
}
void CheckHasNoFavicon(int profile, const GURL& page_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_base::FaviconRawBitmapResult bitmap_result;
favicon_service->GetRawFaviconForPageURL(
page_url, {favicon_base::IconType::kFavicon}, 0,
base::Bind(&OnGotFaviconData, run_loop.QuitClosure(), &bitmap_result),
&task_tracker);
run_loop.Run();
ASSERT_FALSE(bitmap_result.is_valid());
}
void DeleteFaviconMappings(int profile,
const BookmarkNode* node,
FaviconSource favicon_source) {
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.";
if (sync_datatype_helper::test()->use_verifier()) {
const BookmarkNode* v_node = nullptr;
FindNodeInVerifier(model, node, &v_node);
DeleteFaviconMappingsImpl(sync_datatype_helper::test()->verifier(), v_node,
favicon_source);
}
DeleteFaviconMappingsImpl(sync_datatype_helper::test()->GetProfile(profile),
node, favicon_source);
WaitForHistoryToProcessPendingTasks();
}
const BookmarkNode* SetURL(int profile,
......
......@@ -115,6 +115,14 @@ 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);
// Deletes the favicon mappings for |node| in the bookmark model for |profile|.
void DeleteFaviconMappings(int profile,
const bookmarks::BookmarkNode* node,
FaviconSource favicon_source);
// Checks whether |page_url| for |profile| has no favicon mappings.
void CheckHasNoFavicon(int profile, const GURL& page_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,
......
......@@ -22,10 +22,12 @@ using bookmarks::BookmarkModel;
using bookmarks::BookmarkNode;
using bookmarks_helper::AddFolder;
using bookmarks_helper::AddURL;
using bookmarks_helper::CheckHasNoFavicon;
using bookmarks_helper::CountBookmarksWithTitlesMatching;
using bookmarks_helper::CountBookmarksWithUrlsMatching;
using bookmarks_helper::CountFoldersWithTitlesMatching;
using bookmarks_helper::Create1xFaviconFromPNGFile;
using bookmarks_helper::CreateFavicon;
using bookmarks_helper::GetBookmarkBarNode;
using bookmarks_helper::GetBookmarkModel;
using bookmarks_helper::GetOtherNode;
......@@ -267,6 +269,34 @@ IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
EXPECT_TRUE(original_favicon_bytes->Equals(final_favicon_bytes));
}
// Test that a client deletes favicons from sync when they have been removed
// from the local database.
IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest, DeleteFaviconFromSync) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
const GURL page_url("http://www.google.com");
const GURL icon_url("http://www.google.com/favicon.ico");
const BookmarkNode* bookmark = AddURL(kSingleProfileIndex, "title", page_url);
SetFavicon(0, bookmark, icon_url, CreateFavicon(SK_ColorWHITE),
bookmarks_helper::FROM_UI);
ASSERT_TRUE(
UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
// Simulate receiving a favicon deletion from sync.
DeleteFaviconMappings(kSingleProfileIndex, bookmark,
bookmarks_helper::FROM_SYNC);
ASSERT_TRUE(
UpdatedProgressMarkerChecker(GetSyncService(kSingleProfileIndex)).Wait());
ASSERT_TRUE(ModelMatchesVerifier(kSingleProfileIndex));
CheckHasNoFavicon(kSingleProfileIndex, page_url);
EXPECT_TRUE(
GetBookmarkModel(kSingleProfileIndex)->GetFavicon(bookmark).IsEmpty());
}
IN_PROC_BROWSER_TEST_F(SingleClientBookmarksSyncTest,
BookmarkAllNodesRemovedEvent) {
ASSERT_TRUE(SetupClients()) << "SetupClients() failed.";
......
......@@ -37,12 +37,14 @@ using bookmarks_helper::AddURL;
using bookmarks_helper::AllModelsMatch;
using bookmarks_helper::AllModelsMatchVerifier;
using bookmarks_helper::CheckFaviconExpired;
using bookmarks_helper::CheckHasNoFavicon;
using bookmarks_helper::ContainsDuplicateBookmarks;
using bookmarks_helper::CountAllBookmarks;
using bookmarks_helper::CountBookmarksWithTitlesMatching;
using bookmarks_helper::CountBookmarksWithUrlsMatching;
using bookmarks_helper::CountFoldersWithTitlesMatching;
using bookmarks_helper::CreateFavicon;
using bookmarks_helper::DeleteFaviconMappings;
using bookmarks_helper::ExpireFavicon;
using bookmarks_helper::GetBookmarkBarNode;
using bookmarks_helper::GetManagedNode;
......@@ -306,6 +308,35 @@ IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest,
ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
}
IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_DeleteFavicon) {
const GURL page_url("http://www.google.com/a");
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(nullptr, bookmark0);
SetFavicon(0, bookmark0, icon_url, CreateFavicon(SK_ColorWHITE),
bookmarks_helper::FROM_UI);
ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
DeleteFaviconMappings(0, bookmark0, bookmarks_helper::FROM_UI);
ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
// Set the title for |page_url|. This should not revert the deletion of
// favicon mappings.
const char kNewTitle[] = "New Title";
ASSERT_STRNE(kGenericURLTitle, kNewTitle);
const BookmarkNode* bookmark1 = GetUniqueNodeByURL(1, page_url);
SetTitle(1, bookmark1, std::string(kNewTitle));
ASSERT_TRUE(BookmarksMatchVerifierChecker().Wait());
// |page_url| should still have no mapping.
CheckHasNoFavicon(0, page_url);
}
IN_PROC_BROWSER_TEST_F(TwoClientBookmarksSyncTest, SC_AddNonHTTPBMs) {
ASSERT_TRUE(SetupSync()) << "SetupSync() failed.";
ASSERT_TRUE(AllModelsMatchVerifier());
......
......@@ -6,6 +6,7 @@
#include <algorithm>
#include <functional>
#include <string>
#include <utility>
#include "base/bind.h"
......@@ -1025,6 +1026,9 @@ void BookmarkModel::OnFaviconDataAvailable(
// Couldn't load the touch icon, fallback to the regular favicon.
DCHECK(client_->PreferTouchIcon());
LoadFavicon(node, favicon_base::IconType::kFavicon);
} else {
// No favicon available, but we still notify observers.
FaviconLoaded(node);
}
}
......
......@@ -449,10 +449,10 @@ void BookmarkChangeProcessor::BookmarkNodeFaviconChanged(
return;
}
// Ignore changes with empty images. This can happen if the favicon is
// still being loaded.
const gfx::Image& favicon = model->GetFavicon(node);
if (favicon.IsEmpty()) {
// Ignore favicons that are being loaded.
if (!node->is_favicon_loaded()) {
// Sutble way to trigger a load of the favicon.
model->GetFavicon(node);
return;
}
......@@ -844,7 +844,7 @@ const BookmarkNode* BookmarkChangeProcessor::CreateBookmarkNode(
// static
// Sets the favicon of the given bookmark node from the given sync node.
bool BookmarkChangeProcessor::SetBookmarkFavicon(
void BookmarkChangeProcessor::SetBookmarkFavicon(
const syncer::BaseNode* sync_node,
const BookmarkNode* bookmark_node,
BookmarkModel* bookmark_model,
......@@ -852,23 +852,12 @@ bool BookmarkChangeProcessor::SetBookmarkFavicon(
const sync_pb::BookmarkSpecifics& specifics =
sync_node->GetBookmarkSpecifics();
const std::string& icon_bytes_str = specifics.favicon();
if (icon_bytes_str.empty())
return false;
scoped_refptr<base::RefCountedString> icon_bytes(
new base::RefCountedString());
icon_bytes->data().assign(icon_bytes_str);
GURL icon_url(specifics.icon_url());
// Old clients may not be syncing the favicon URL. If the icon URL is not
// synced, use the page URL as a fake icon URL as it is guaranteed to be
// unique.
if (icon_url.is_empty())
icon_url = bookmark_node->url();
ApplyBookmarkFavicon(bookmark_node, sync_client, icon_url, icon_bytes);
return true;
ApplyBookmarkFavicon(bookmark_node, sync_client, GURL(specifics.icon_url()),
icon_bytes);
}
// static
......@@ -943,14 +932,36 @@ void BookmarkChangeProcessor::ApplyBookmarkFavicon(
history::HistoryService* history = sync_client->GetHistoryService();
favicon::FaviconService* favicon_service = sync_client->GetFaviconService();
// Some tests (that use FakeSyncClient) use no services.
if (history == nullptr)
return;
DCHECK(favicon_service);
history->AddPageNoVisitForBookmark(bookmark_node->url(),
bookmark_node->GetTitle());
GURL icon_url_to_use = icon_url;
if (icon_url.is_empty()) {
if (bitmap_data->size() == 0) {
// Empty icon URL and no bitmap data means no icon mapping.
favicon_service->DeleteFaviconMappings({bookmark_node->url()},
favicon_base::IconType::kFavicon);
return;
} else {
// Ancient clients (prior to M25) may not be syncing the favicon URL. If
// the icon URL is not synced, use the page URL as a fake icon URL as it
// is guaranteed to be unique.
icon_url_to_use = bookmark_node->url();
}
}
// The client may have cached the favicon at 2x. Use MergeFavicon() as not to
// overwrite the cached 2x favicon bitmap. Sync favicons are always
// gfx::kFaviconSize in width and height. Store the favicon into history
// as such.
gfx::Size pixel_size(gfx::kFaviconSize, gfx::kFaviconSize);
favicon_service->MergeFavicon(bookmark_node->url(), icon_url,
favicon_service->MergeFavicon(bookmark_node->url(), icon_url_to_use,
favicon_base::IconType::kFavicon, bitmap_data,
pixel_size);
}
......@@ -962,16 +973,21 @@ void BookmarkChangeProcessor::SetSyncNodeFavicon(
syncer::WriteNode* sync_node) {
scoped_refptr<base::RefCountedMemory> favicon_bytes(nullptr);
EncodeFavicon(bookmark_node, model, &favicon_bytes);
sync_pb::BookmarkSpecifics updated_specifics(
sync_node->GetBookmarkSpecifics());
if (favicon_bytes.get() && favicon_bytes->size()) {
sync_pb::BookmarkSpecifics updated_specifics(
sync_node->GetBookmarkSpecifics());
updated_specifics.set_favicon(favicon_bytes->front(),
favicon_bytes->size());
updated_specifics.set_icon_url(bookmark_node->icon_url()
? bookmark_node->icon_url()->spec()
: std::string());
sync_node->SetBookmarkSpecifics(updated_specifics);
} else {
updated_specifics.clear_favicon();
updated_specifics.clear_icon_url();
}
sync_node->SetBookmarkSpecifics(updated_specifics);
}
bool BookmarkChangeProcessor::CanSyncNode(const BookmarkNode* node) {
......
......@@ -119,10 +119,9 @@ class BookmarkChangeProcessor : public bookmarks::BookmarkModelObserver,
int index);
// Sets the favicon of the given bookmark node from the given sync node.
// Returns whether the favicon was set in the bookmark node.
// |profile| is the profile that contains the HistoryService and BookmarkModel
// for the bookmark in question.
static bool SetBookmarkFavicon(const syncer::BaseNode* sync_node,
static void SetBookmarkFavicon(const syncer::BaseNode* sync_node,
const bookmarks::BookmarkNode* bookmark_node,
bookmarks::BookmarkModel* model,
syncer::SyncClient* sync_client);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment