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..59b90e45cd29277d76a288269c37c8f8026d9d62 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,25 +435,43 @@ void BookmarkModel::SetNodeSyncTransactionVersion(
     store_->ScheduleSave();
 }
 
-void BookmarkModel::OnFaviconChanged(const std::set<GURL>& urls) {
+void BookmarkModel::OnFaviconsChanged(const std::set<GURL>& page_urls,
+                                      const std::set<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)

[sky, 2015/06/22 15:19:48]

to_update.insert(nodes.begin(), nodes.end())

[pkotwicz, 2015/06/26 15:14:28]

Done.

+      to_update.insert(node);
+  }
+
+  if (!icon_urls.empty()) {
+    // Log Histogram to determine how often |icon_urls| is non empty in
+    // practice.
+    // TODO(pkotwicz): Do something more efficient if |icon_urls| is non-empty
+    // many times a day for each user.
+    UMA_HISTOGRAM_BOOLEAN("Bookmarks.OnFaviconsChangedMultipleIconURLs", true);

[sky, 2015/06/22 15:19:48]

Can you also include the size of icon_urls?

[pkotwicz, 2015/06/26 15:14:28]

|icon_urls| has at most one element. Do you want me to make the histogram take
in the size of |icon_urls| in case this changes?
My hope is to deprecate the histogram a couple of months after this CL lands

[sky, 2015/06/29 14:51:03]

In that case why do we need a set? Why not a single url and use an empty url to
indicate none?

+
+    base::AutoLock url_lock(url_lock_);
+    for (const BookmarkNode* node : nodes_ordered_by_url_set_) {
+      if (icon_urls.count(node->icon_url()))
+        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));
+  }
 }
 
 void BookmarkModel::SetDateAdded(const BookmarkNode* node, Time date_added) {