Make sure to update the BookmarkIndex on a URL Change

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"
@@ skipping 353 matching lines @@
 
   BookmarkNode* mutable_node = AsMutable(node);
   mutable_node->InvalidateFavicon();
   CancelPendingFaviconLoadRequests(mutable_node);
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     OnWillChangeBookmarkNode(this, node));
 
   {
     base::AutoLock url_lock(url_lock_);
-    RemoveNodeFromURLSet(mutable_node);
+    RemoveNodeFromInternalMaps(mutable_node);
     mutable_node->set_url(url);
-    nodes_ordered_by_url_set_.insert(mutable_node);
+    AddNodeToInternalMaps(mutable_node);
   }
 
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeChanged(this, node));
 }
 
 void BookmarkModel::SetNodeMetaInfo(const BookmarkNode* node,
@@ skipping 223 matching lines @@
   if (creation_time > parent->date_folder_modified())
     SetDateFolderModified(parent, creation_time);
 
   BookmarkNode* new_node = new BookmarkNode(generate_next_node_id(), url);
   new_node->SetTitle(title);
   new_node->set_date_added(creation_time);
   new_node->set_type(BookmarkNode::URL);
   if (meta_info)
     new_node->SetMetaInfoMap(*meta_info);
 
-  {
-    // Only hold the lock for the duration of the insert.
-    base::AutoLock url_lock(url_lock_);
-    nodes_ordered_by_url_set_.insert(new_node);
-  }
-
   return AddNode(AsMutable(parent), index, new_node);
 }
 
 void BookmarkModel::SortChildren(const BookmarkNode* parent) {
   DCHECK(client_->CanBeEditedByUser(parent));
 
   if (!parent || !parent->is_folder() || is_root_node(parent) ||
       parent->child_count() <= 1) {
     return;
   }
@@ skipping 97 matching lines @@
 
 void BookmarkModel::RemoveNode(BookmarkNode* node,
                                std::set<GURL>* removed_urls) {
   if (!loaded_ || !node || is_permanent_node(node)) {
     NOTREACHED();
     return;
   }
 
   url_lock_.AssertAcquired();
   if (node->is_url()) {
-    RemoveNodeFromURLSet(node);
+    RemoveNodeFromInternalMaps(node);
     removed_urls->insert(node->url());
-    index_->Remove(node);
   }
 
   CancelPendingFaviconLoadRequests(node);
 
   // Recurse through children.
   for (int i = node->child_count() - 1; i >= 0; --i)
     RemoveNode(node->GetChild(i), removed_urls);
 }
 
 void BookmarkModel::DoneLoading(scoped_ptr<BookmarkLoadDetails> details) {
@@ skipping 74 matching lines @@
 
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(
       BookmarkModelObserver,
       observers_,
       BookmarkNodeRemoved(this, parent, index, node.get(), removed_urls));
 }
 
-void BookmarkModel::RemoveNodeFromURLSet(BookmarkNode* node) {
+void BookmarkModel::RemoveNodeFromInternalMaps(BookmarkNode* node) {
+  index_->Remove(node);
   // NOTE: this is called in such a way that url_lock_ is already held. As
   // such, this doesn't explicitly grab the lock.
   NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(node);
   DCHECK(i != nodes_ordered_by_url_set_.end());
   // i points to the first node with the URL, advance until we find the
   // node we're removing.
   while (*i != node)
     ++i;
   nodes_ordered_by_url_set_.erase(i);
 }
@@ skipping 28 matching lines @@
                                      int index,
                                      BookmarkNode* node) {
   parent->Add(node, index);
 
   if (store_.get())
     store_->ScheduleSave();
 
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeAdded(this, parent, index));
 
-  index_->Add(node);
+  if (node->type() == BookmarkNode::URL) {

[danduong, 2014/11/04 01:48:06]

Is this right? It seems to match the previous logic closest.

[sky, 2014/11/04 03:41:45]

I don't think there is a good reason why this is after notifying observers. I
think you should move your code to 879ish. And yes, this looks right.

[danduong, 2014/11/04 03:52:48]

Done.

+    base::AutoLock url_lock(url_lock_);
+    AddNodeToInternalMaps(node);

[danduong, 2014/11/04 01:48:05]

Moving this here actually makes you hold on to url_lock_ slightly longer for the
index_->Add call. Is that OK?

[sky, 2014/11/04 03:41:45]

I don't think it'll make much of a difference.

+  } else {
+    index_->Add(node);
+  }
 
   return node;
 }
 
+void BookmarkModel::AddNodeToInternalMaps(BookmarkNode* node) {
+  index_->Add(node);
+  nodes_ordered_by_url_set_.insert(node);
+}
+
 bool BookmarkModel::IsValidIndex(const BookmarkNode* parent,
                                  int index,
                                  bool allow_end) {
   return (parent && parent->is_folder() &&
           (index >= 0 && (index < parent->child_count() ||
                           (allow_end && index == parent->child_count()))));
 }
 
 BookmarkPermanentNode* BookmarkModel::CreatePermanentNode(
     BookmarkNode::Type type) {
@@ skipping 100 matching lines @@
   BookmarkPermanentNode* mobile_node =
       CreatePermanentNode(BookmarkNode::MOBILE);
   return scoped_ptr<BookmarkLoadDetails>(new BookmarkLoadDetails(
       bb_node,
       other_node,
       mobile_node,
       client_->GetLoadExtraNodesCallback(),
       new BookmarkIndex(client_, accept_languages),
       next_node_id_));
 }