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

components/bookmarks/browser/bookmark_model.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "components/bookmarks/browser/bookmark_model.h"
 
 #include <algorithm>
 #include <functional>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #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"
 #include "components/bookmarks/browser/bookmark_index.h"
 #include "components/bookmarks/browser/bookmark_match.h"
 #include "components/bookmarks/browser/bookmark_model_observer.h"
 #include "components/bookmarks/browser/bookmark_node_data.h"
 #include "components/bookmarks/browser/bookmark_storage.h"
 #include "components/bookmarks/browser/bookmark_utils.h"
 #include "components/favicon_base/favicon_types.h"
@@ skipping 402 matching lines @@
   DCHECK(client_->CanSyncNode(node));
 
   if (sync_transaction_version == node->sync_transaction_version())
     return;
 
   AsMutable(node)->set_sync_transaction_version(sync_transaction_version);
   if (store_.get())
     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) {
   DCHECK(node && !is_permanent_node(node));
 
   if (node->date_added() == date_added)
     return;
 
   AsMutable(node)->set_date_added(date_added);
 
@@ skipping 562 matching lines @@
   return scoped_ptr<BookmarkLoadDetails>(new BookmarkLoadDetails(
       bb_node,
       other_node,
       mobile_node,
       client_->GetLoadExtraNodesCallback(),
       new BookmarkIndex(client_, accept_languages),
       next_node_id_));
 }
 
 }  // namespace bookmarks