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 cf969748c6fb5b5751c50bfade02a6d224f790d7..d8c466f73cc2ba3a4569cb61aefc943183bdcf4a 100644
--- a/components/bookmarks/browser/bookmark_model.cc
+++ b/components/bookmarks/browser/bookmark_model.cc
@@ -12,6 +12,7 @@
 #include "base/i18n/string_compare.h"
 #include "base/logging.h"
 #include "base/macros.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/strings/string_util.h"
 #include "components/bookmarks/browser/bookmark_expanded_state_tracker.h"
@@ -434,24 +435,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, node));
   }
 }
 
@@ -483,6 +495,20 @@ void BookmarkModel::GetNodesByURL(const GURL& url,
   }
 }
 
+void BookmarkModel::GetNodesByIconURL(const GURL& icon_url,
+                                      std::vector<const BookmarkNode*>* nodes) {
+  // Log Histogram to determine how often this method is called in practice.

[sky, 2015/06/19 16:38:02]

WDYT of moving this to OnFaviconsChanged? What we care about there is both how
often it's called, and how many nodes are updated.

+  // TODO(pkotwicz): Do something more efficient if this method gets called many
+  // times a day for each user.
+  UMA_HISTOGRAM_BOOLEAN("Bookmarks.GetNodesByIconURL", true);
+
+  base::AutoLock url_lock(url_lock_);
+  for (const BookmarkNode* node : nodes_ordered_by_url_set_) {

[sky, 2015/06/19 16:38:02]

I prefer inlining this in OnFaviconsChanged and changing the ordering so you
loop through all the nodes seeing if the icon url of the node is in icon_urls.

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