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 d3ac9e3830a51b1f3faf9cbe54e2ea05c735f3ca..5709533dba2d0be60f7f29ded432599ba10be75a 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,21 +435,38 @@ void BookmarkModel::SetNodeSyncTransactionVersion(
     store_->ScheduleSave();
 }
 
-void BookmarkModel::OnFaviconChanged(const std::set<GURL>& urls) {
-  // Prevent the observers from getting confused for multiple favicon loads.
-  for (std::set<GURL>::const_iterator i = urls.begin(); i != urls.end(); ++i) {
+void BookmarkModel::OnFaviconsChanged(const std::set<GURL>& page_urls,
+                                      const GURL& icon_url) {
+  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);
+    to_update.insert(nodes.begin(), nodes.end());
+  }
+
+  if (!icon_url.is_empty()) {
+    // Log Histogram to determine how often |icon_url| is non empty in
+    // practice.
+    // TODO(pkotwicz): Do something more efficient if |icon_url| is non-empty
+    // many times a day for each user.
+    UMA_HISTOGRAM_BOOLEAN("Bookmarks.OnFaviconsChangedIconURL", true);

[Ilya Sherman, 2015/07/06 17:57:40]

I'd recommend building in a baseline for this histogram, so that you have some
context for the numbers you see on the dashboard.

[pkotwicz, 2015/07/06 18:47:30]

How would I build a baseline?

+
+    base::AutoLock url_lock(url_lock_);
+    for (const BookmarkNode* node : nodes_ordered_by_url_set_) {
+      if (icon_url == 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) {