Update all bookmarks which use an icon URL when a favicon's bitmap is updated

components/bookmarks/browser/bookmark_model.cc

Index: components/bookmarks/browser/bookmark_model.cc
diff --git a/components/bookmarks/browser/bookmark_model.cc b/components/bookmarks/browser/bookmark_model.cc
index 2e174dbedc63314fc2de85d1accdda9582439c90..6f65af880117d935637a261cf43dfe42c224c3da 100644
--- a/components/bookmarks/browser/bookmark_model.cc
+++ b/components/bookmarks/browser/bookmark_model.cc
@@ -446,24 +446,35 @@ void BookmarkModel::SetNodeSyncTransactionVersion(
     store_->ScheduleSave();
 }
 
-void BookmarkModel::OnFaviconChanged(const std::set<GURL>& urls) {
+void BookmarkModel::OnFaviconsChanged(const std::vector<GURL>& page_urls,
+                                      const std::vector<GURL>& icon_urls) {
   // Ignore events if |Load| has not been called yet.
   if (!store_)
     return;
 
-  // Prevent the observers from getting confused for multiple favicon loads.
-  for (std::set<GURL>::const_iterator i = urls.begin(); i != urls.end(); ++i) {
+  std::set<const BookmarkNode*> to_update;
+  for (const GURL& page_url : page_urls) {
     std::vector<const BookmarkNode*> nodes;
-    GetNodesByURL(*i, &nodes);
-    for (size_t i = 0; i < nodes.size(); ++i) {
-      // Got an updated favicon, for a URL, do a new request.
-      BookmarkNode* node = AsMutable(nodes[i]);
-      node->InvalidateFavicon();
-      CancelPendingFaviconLoadRequests(node);
-      FOR_EACH_OBSERVER(BookmarkModelObserver,
-                        observers_,
-                        BookmarkNodeFaviconChanged(this, node));
-    }
+    GetNodesByURL(page_url, &nodes);
+    for (const BookmarkNode* node : nodes)
+      to_update.insert(node);
+  }
+
+  for (const GURL& icon_url : icon_urls) {
+    std::vector<const BookmarkNode*> nodes;
+    GetNodesByIconURL(icon_url, &nodes);
+    for (const BookmarkNode* node : nodes)
+      to_update.insert(node);
+  }
+
+  for (const BookmarkNode* node : to_update) {
+    // Rerequest the favicon.
+    BookmarkNode* mutable_node = AsMutable(node);
+    mutable_node->InvalidateFavicon();
+    CancelPendingFaviconLoadRequests(mutable_node);
+    FOR_EACH_OBSERVER(BookmarkModelObserver,
+                      observers_,
+                      BookmarkNodeFaviconChanged(this, mutable_node));
   }
 }
 
@@ -504,6 +515,16 @@ void BookmarkModel::GetNodesByURL(const GURL& url,
   }
 }
 
+void BookmarkModel::GetNodesByIconURL(const GURL& icon_url,
+                                      std::vector<const BookmarkNode*>* nodes) {
+  // TODO: Do something more efficient if this gets called a lot

[sky, 2015/05/22 03:09:06]

Any idea how often this will be called? Might a sync trigger this being called a
lot?

[pkotwicz, 2015/05/22 16:00:39]

Unfortunately, this is called more often than I expected. If a user has two
different devices which sync with each other then this can be called quite a
bit.
The favicon PNG data which is stored in the database is different when it is
encoded on iOS, Mac, Linux and Android (Android picks the largest bitmap of an
.ico and scales it down to 16x16)

GetNodesByIconURL() may be called for several icon URLs each time that a user
switches the device that they use

I am unsure whether this necessitates improving GetNodesByIconURL()

Unfortunately, fixing the Android behavior is hard (I hoped that the work that
rogerm@ was doing would fix it but he has moved on to another project)

+  base::AutoLock url_lock(url_lock_);
+  for (const BookmarkNode* node : nodes_ordered_by_url_set_) {
+    if (node->icon_url() == icon_url)
+      nodes->push_back(node);
+  }
+}
+
 const BookmarkNode* BookmarkModel::GetMostRecentlyAddedUserNodeForURL(
     const GURL& url) {
   std::vector<const BookmarkNode*> nodes;