Implement BookmarkModel::SetUrl

BUG=10603
TEST=*_test.exe --gtest_filter=*Bookmark*
     Also manually tested changing the URL through the bookmarks extension API, bookmark manager, bookmark bar and saw it update in all places and correctly sync to another chrome instance.

Review URL: http://codereview.chromium.org/582022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@38509 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 79 insertions, 49 deletions

diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index 5385810..f32ac74 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -248,6 +248,46 @@ void BookmarkModel::SetTitle(const BookmarkNode* node, const string16& title) {
                     BookmarkNodeChanged(this, node));
 }
 
+void BookmarkModel::SetURL(const BookmarkNode* node, const GURL& url) {
+  if (!node) {
+    NOTREACHED();
+    return;
+  }
+
+  // We cannot change the URL of a folder.
+  if (node->is_folder()) {
+    NOTREACHED();
+    return;
+  }
+
+  if (url == node->GetURL())
+    return;
+
+  AsMutable(node)->InvalidateFavicon();
+  CancelPendingFavIconLoadRequests(AsMutable(node));
+
+  {
+    AutoLock url_lock(url_lock_);
+    NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(
+        AsMutable(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);
+
+    AsMutable(node)->SetURL(url);
+    nodes_ordered_by_url_set_.insert(AsMutable(node));
+  }
+
+  if (store_.get())
+    store_->ScheduleSave();
+
+  FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
+                    BookmarkNodeChanged(this, node));
+}
+
 void BookmarkModel::GetNodesByURL(const GURL& url,
                                   std::vector<const BookmarkNode*>* nodes) {
   AutoLock url_lock(url_lock_);
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index 1ffdcbe..7790790 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -61,6 +61,8 @@ class BookmarkNode : public TreeNode<BookmarkNode> {
 
   // Returns the URL.
   const GURL& GetURL() const { return url_; }
+  // Sets the URL to the given value.
+  void SetURL(const GURL& url) { url_ = url; }
 
   // Returns a unique id for this node.
   // For bookmark nodes that are managed by the bookmark model, the IDs are
@@ -144,8 +146,8 @@ class BookmarkNode : public TreeNode<BookmarkNode> {
   HistoryService::Handle favicon_load_handle_;
 
   // The URL. BookmarkModel maintains maps off this URL, it is important that
-  // it not change once the node has been created.
-  const GURL url_;
+  // changes to the URL is done through the bookmark model.
+  GURL url_;
 
   // Type of node.
   BookmarkNode::Type type_;
@@ -231,6 +233,9 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
 #endif
   void SetTitle(const BookmarkNode* node, const string16& title);
 
+  // Sets the URL of the specified bookmark node.
+  void SetURL(const BookmarkNode* node, const GURL& url);
+
   // Returns true if the model finished loading.
   bool IsLoaded() { return loaded_; }
 
diff --git a/chrome/browser/bookmarks/bookmark_model_observer.h b/chrome/browser/bookmarks/bookmark_model_observer.h
index 2136475..7cc50dc 100644
--- a/chrome/browser/bookmarks/bookmark_model_observer.h
+++ b/chrome/browser/bookmarks/bookmark_model_observer.h
@@ -39,7 +39,7 @@ class BookmarkModelObserver {
                                    int old_index,
                                    const BookmarkNode* node) = 0;
 
-  // Invoked when the title or favicon of a node has changed.
+  // Invoked when the title, url or favicon of a node has changed.
   virtual void BookmarkNodeChanged(BookmarkModel* model,
                                    const BookmarkNode* node) = 0;
 
diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc
index 0624b74..2200f4e 100644
--- a/chrome/browser/bookmarks/bookmark_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc
@@ -264,6 +264,21 @@ TEST_F(BookmarkModelTest, SetTitle) {
   EXPECT_EQ(title, node->GetTitle());
 }
 
+TEST_F(BookmarkModelTest, SetURL) {
+  const BookmarkNode* root = model.GetBookmarkBarNode();
+  const std::wstring title(L"foo");
+  GURL url("http://foo.com");
+  const BookmarkNode* node = model.AddURL(root, 0, title, url);
+
+  ClearCounts();
+
+  url = GURL("http://foo2.com");
+  model.SetURL(node, url);
+  AssertObserverCount(0, 0, 0, 1, 0);
+  observer_details.AssertEquals(node, NULL, -1, -1);
+  EXPECT_EQ(url, node->GetURL());
+}
+
 TEST_F(BookmarkModelTest, Move) {
   const BookmarkNode* root = model.GetBookmarkBarNode();
   std::wstring title(L"foo");
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc
index a172312..c6f82b3 100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -529,24 +529,11 @@ const BookmarkNode* ApplyEditsWithNoGroupChange(BookmarkModel* model,
 
   const BookmarkNode* node = details.existing_node;
   DCHECK(node);
-  const BookmarkNode* old_parent = node->GetParent();
-  int old_index = old_parent ? old_parent->IndexOfChild(node) : -1;
 
-  // If we're not showing the tree we only need to modify the node.
-  if (old_index == -1) {
-    NOTREACHED();
-    return node;
-  }
+  if (node->is_url())
+    model->SetURL(node, new_url);
+  model->SetTitle(node, new_title);
 
-  if (new_url != node->GetURL()) {
-    // TODO(sky): need SetURL on the model.
-    const BookmarkNode* new_node = model->AddURLWithCreationTime(old_parent,
-        old_index, new_title, new_url, node->date_added());
-    model->Remove(old_parent, old_index + 1);
-    return new_node;
-  } else {
-    model->SetTitle(node, new_title);
-  }
   return node;
 }
 
@@ -562,31 +549,14 @@ const BookmarkNode* ApplyEditsWithPossibleGroupChange(BookmarkModel* model,
 
   const BookmarkNode* node = details.existing_node;
   DCHECK(node);
-  const BookmarkNode* old_parent = node->GetParent();
-  int old_index = old_parent->IndexOfChild(node);
-  const BookmarkNode* return_node = node;
-
-  Time date_added = node->date_added();
-  if (new_parent == node->GetParent()) {
-    // The parent is the same.
-    if (node->is_url() && new_url != node->GetURL()) {
-      model->Remove(old_parent, old_index);
-      return_node = model->AddURLWithCreationTime(old_parent, old_index,
-          new_title, new_url, date_added);
-    } else {
-      model->SetTitle(node, new_title);
-    }
-  } else if (node->is_url() && new_url != node->GetURL()) {
-    // The parent and URL changed.
-    model->Remove(old_parent, old_index);
-    return_node = model->AddURLWithCreationTime(new_parent,
-        new_parent->GetChildCount(), new_title, new_url, date_added);
-  } else {
-    // The parent and title changed. Move the node and change the title.
+
+  if (new_parent != node->GetParent())
     model->Move(node, new_parent, new_parent->GetChildCount());
-    model->SetTitle(node, new_title);
-  }
-  return return_node;
+  if (node->is_url())
+    model->SetURL(node, new_url);
+  model->SetTitle(node, new_title);
+
+  return node;
 }
 
 // Formerly in BookmarkBarView
diff --git a/chrome/browser/bookmarks/bookmark_utils.h b/chrome/browser/bookmarks/bookmark_utils.h
index 7117967..0bea122 100644
--- a/chrome/browser/bookmarks/bookmark_utils.h
+++ b/chrome/browser/bookmarks/bookmark_utils.h
@@ -149,9 +149,9 @@ bool DoesBookmarkContainText(const BookmarkNode* node,
                              const std::wstring& languages);
 
 // Modifies a bookmark node (assuming that there's no magic that needs to be
-// done regarding moving from one folder to another).  If the URL changed or a
-// new node is explicitly being added, returns a pointer to the new node that
-// was created.  Otherwise the return value is identically |node|.
+// done regarding moving from one folder to another).  If a new node is
+// explicitly being added, returns a pointer to the new node that was created.
+// Otherwise the return value is identically |node|.
 const BookmarkNode* ApplyEditsWithNoGroupChange(
     BookmarkModel* model,
     const BookmarkNode* parent,
@@ -161,9 +161,9 @@ const BookmarkNode* ApplyEditsWithNoGroupChange(
     BookmarkEditor::Handler* handler);
 
 // Modifies a bookmark node assuming that the parent of the node may have
-// changed and the node will need to be removed and reinserted.  If the URL
-// changed or a new node is explicitly being added, returns a pointer to the
-// new node that was created.  Otherwise the return value is identically |node|.
+// changed and the node will need to be removed and reinserted.  If a new node
+// is explicitly being added, returns a pointer to the new node that was
+// created.  Otherwise the return value is identically |node|.
 const BookmarkNode* ApplyEditsWithPossibleGroupChange(
     BookmarkModel* model,
     const BookmarkNode* new_parent,