Changes the bookmark model to allow more than one bookmark to

reference the same url. Clicking the star button edits the most
recently added bookmark for the URL. Dragging a button/star always
does a move, otherwise drops on the bookmark bar create a new
bookmark.

Also changed the add page context menu for the bookmark bar to
remember where you invoked it from.

BUG=1173228 1678

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

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

7 files changed, 267 insertions, 94 deletions

diff --git a/chrome/browser/bookmarks/bookmark_drag_data.cc b/chrome/browser/bookmarks/bookmark_drag_data.cc
index 996470c..d2ff684 100644
--- a/chrome/browser/bookmarks/bookmark_drag_data.cc
+++ b/chrome/browser/bookmarks/bookmark_drag_data.cc
@@ -6,6 +6,7 @@
 
 #include "base/pickle.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
+#include "chrome/browser/profile.h"
 #include "chrome/common/os_exchange_data.h"
 
 static CLIPFORMAT clipboard_format = 0;
@@ -30,15 +31,16 @@ BookmarkDragData::BookmarkDragData(BookmarkNode* node)
     AddChildren(node);
 }
 
-void BookmarkDragData::Write(OSExchangeData* data) const {
+void BookmarkDragData::Write(Profile* profile, OSExchangeData* data) const {
   RegisterFormat();
 
   DCHECK(data);
 
-  if (is_url) {
+  if (is_url)
     data->SetURL(url, title);
-  }
   Pickle data_pickle;
+  data_pickle.WriteWString(profile->GetPath());
+  data_pickle.WriteInt(id_);
   WriteToPickle(&data_pickle);
   data->SetPickledData(clipboard_format, data_pickle);
 }
@@ -48,13 +50,15 @@ bool BookmarkDragData::Read(const OSExchangeData& data) {
 
   is_valid = data.GetURLAndTitle(&url, &title) && url.is_valid();
   is_url = is_valid;
-  profile_id.clear();
+  profile_path_.clear();
 
   if (data.HasFormat(clipboard_format)) {
     Pickle drag_data_pickle;
     if (data.GetPickledData(clipboard_format, &drag_data_pickle)) {
       void* data_iterator = NULL;
-      if (ReadFromPickle(&drag_data_pickle, &data_iterator)) {
+      if (drag_data_pickle.ReadWString(&data_iterator, &profile_path_) &&
+          drag_data_pickle.ReadInt(&data_iterator, &id_) &&
+          ReadFromPickle(&drag_data_pickle, &data_iterator)) {
         is_valid = true;
       }
     }
@@ -62,18 +66,17 @@ bool BookmarkDragData::Read(const OSExchangeData& data) {
   return is_valid;
 }
 
-BookmarkNode* BookmarkDragData::GetNode(BookmarkModel* model) const {
-  DCHECK(!is_url && id_ && is_valid);
-  return model->GetNodeByID(id_);
+BookmarkNode* BookmarkDragData::GetNode(Profile* profile) const {
+  DCHECK(is_valid);
+  return (profile->GetPath() == profile_path_) ?
+      profile->GetBookmarkModel()->GetNodeByID(id_) : NULL;
 }
 
 void BookmarkDragData::WriteToPickle(Pickle* pickle) const {
   pickle->WriteBool(is_url);
-  pickle->WriteWString(profile_id);
   pickle->WriteString(url.spec());
   pickle->WriteWString(title);
   if (!is_url) {
-    pickle->WriteInt(id_);
     pickle->WriteInt(static_cast<int>(children.size()));
     for (std::vector<BookmarkDragData>::const_iterator i = children.begin();
          i != children.end(); ++i) {
@@ -86,17 +89,13 @@ bool BookmarkDragData::ReadFromPickle(Pickle* pickle, void** iterator) {
   std::string url_spec;
   is_valid = false;
   if (!pickle->ReadBool(iterator, &is_url) ||
-      !pickle->ReadWString(iterator, &profile_id) ||
       !pickle->ReadString(iterator, &url_spec) ||
       !pickle->ReadWString(iterator, &title)) {
     return false;
   }
   url = GURL(url_spec);
   if (!is_url) {
-    id_ = 0;
     children.clear();
-    if (!pickle->ReadInt(iterator, &id_))
-      return false;
     int children_count;
     if (!pickle->ReadInt(iterator, &children_count))
       return false;
@@ -115,4 +114,3 @@ void BookmarkDragData::AddChildren(BookmarkNode* node) {
   for (int i = 0, max = node->GetChildCount(); i < max; ++i)
     children.push_back(BookmarkDragData(node->GetChild(i)));
 }
-
diff --git a/chrome/browser/bookmarks/bookmark_drag_data.h b/chrome/browser/bookmarks/bookmark_drag_data.h
index c7e1fed..564393a 100644
--- a/chrome/browser/bookmarks/bookmark_drag_data.h
+++ b/chrome/browser/bookmarks/bookmark_drag_data.h
@@ -14,6 +14,7 @@ class BookmarkModel;
 class BookmarkNode;
 class OSExchangeData;
 class Pickle;
+class Profile;
 
 // BookmarkDragData is used by the bookmark bar to represent a dragged
 // URL or starred group on the clipboard during drag and drop.
@@ -36,23 +37,19 @@ struct BookmarkDragData {
 
   // Writes this BookmarkDragData to data. If BookmarkDragData is a URL,
   // this writes out the URL and URL title clipboard data as well.
-  void Write(OSExchangeData* data) const;
+  void Write(Profile* profile, OSExchangeData* data) const;
 
   // Restores this data from the clipboard, returning true on success.
   bool Read(const OSExchangeData& data);
 
-  // Returns the node represented by this drag data from root. If the
-  // path can not be found, NULL is returned.
-  //
-  // This is only valid for groups.
-  BookmarkNode* BookmarkDragData::GetNode(BookmarkModel* model) const;
+  // Returns the node represented by this DragData. If this DragData was created
+  // from the same profile then the node from the model is returned. If the
+  // node can't be found (may have been deleted), NULL is returned.
+  BookmarkNode* BookmarkDragData::GetNode(Profile* profile) const;
 
   // If true, this entry represents a StarredEntry of type URL.
   bool is_url;
 
-  // ID of the profile we originated from.
-  std::wstring profile_id;
-
   // The URL, only valid if is_url is true.
   GURL url;
 
@@ -74,7 +71,12 @@ struct BookmarkDragData {
   // Adds to children an entry for each child of node.
   void AddChildren(BookmarkNode* node);
 
+  // Path of the profile we originated from.
+  // This is only saved for the root node.
+  std::wstring profile_path_;
+
   // ID (node->id()) of the node this BookmarkDragData was created from.
+  // This is only saved for the root node.
   int id_;
 };
 
diff --git a/chrome/browser/bookmarks/bookmark_drag_data_unittest.cc b/chrome/browser/bookmarks/bookmark_drag_data_unittest.cc
index 76f0f67..a3022f8 100644
--- a/chrome/browser/bookmarks/bookmark_drag_data_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_drag_data_unittest.cc
@@ -6,6 +6,7 @@
 #include "chrome/browser/bookmarks/bookmark_drag_data.h"
 #include "chrome/browser/bookmarks/bookmark_model.h"
 #include "chrome/common/os_exchange_data.h"
+#include "chrome/test/testing_profile.h"
 #include "googleurl/src/gurl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -25,20 +26,21 @@ TEST_F(BookmarkDragDataTest, BogusRead) {
 }
 
 TEST_F(BookmarkDragDataTest, URL) {
-  BookmarkModel model(NULL);
-  BookmarkNode* root = model.GetBookmarkBarNode();
+  TestingProfile profile;
+  profile.CreateBookmarkModel(false);
+  profile.SetID(L"id");
+  BookmarkModel* model = profile.GetBookmarkModel();
+  BookmarkNode* root = model->GetBookmarkBarNode();
   GURL url(GURL("http://foo.com"));
-  const std::wstring profile_id(L"blah");
   const std::wstring title(L"blah");
-  BookmarkNode* node = model.AddURL(root, 0, title, url);
+  BookmarkNode* node = model->AddURL(root, 0, title, url);
   BookmarkDragData drag_data(node);
-  drag_data.profile_id = profile_id;
   EXPECT_TRUE(drag_data.url == url);
   EXPECT_EQ(title, drag_data.title);
   EXPECT_TRUE(drag_data.is_url);
 
   scoped_refptr<OSExchangeData> data(new OSExchangeData());
-  drag_data.Write(data.get());
+  drag_data.Write(&profile, data.get());
 
   // Now read the data back in.
   scoped_refptr<OSExchangeData> data2(new OSExchangeData(data.get()));
@@ -48,6 +50,10 @@ TEST_F(BookmarkDragDataTest, URL) {
   EXPECT_EQ(title, read_data.title);
   EXPECT_TRUE(read_data.is_valid);
   EXPECT_TRUE(read_data.is_url);
+  EXPECT_TRUE(read_data.GetNode(&profile) == node);
+
+  TestingProfile profile2(1);
+  EXPECT_TRUE(read_data.GetNode(&profile2) == NULL);
 
   // Writing should also put the URL and title on the clipboard.
   GURL read_url;
@@ -58,50 +64,54 @@ TEST_F(BookmarkDragDataTest, URL) {
 }
 
 TEST_F(BookmarkDragDataTest, Group) {
-  BookmarkModel model(NULL);
-  BookmarkNode* root = model.GetBookmarkBarNode();
-  BookmarkNode* g1 = model.AddGroup(root, 0, L"g1");
-  BookmarkNode* g11 = model.AddGroup(g1, 0, L"g11");
-  BookmarkNode* g12 = model.AddGroup(g1, 0, L"g12");
+  TestingProfile profile;
+  profile.CreateBookmarkModel(false);
+  profile.SetID(L"id");
+  BookmarkModel* model = profile.GetBookmarkModel();
+  BookmarkNode* root = model->GetBookmarkBarNode();
+  BookmarkNode* g1 = model->AddGroup(root, 0, L"g1");
+  BookmarkNode* g11 = model->AddGroup(g1, 0, L"g11");
+  BookmarkNode* g12 = model->AddGroup(g1, 0, L"g12");
 
   BookmarkDragData drag_data(g12);
-  const std::wstring profile_id(L"blah");
-  drag_data.profile_id = profile_id;
   EXPECT_EQ(g12->GetTitle(), drag_data.title);
   EXPECT_FALSE(drag_data.is_url);
 
   scoped_refptr<OSExchangeData> data(new OSExchangeData());
-  drag_data.Write(data.get());
+  drag_data.Write(&profile, data.get());
 
   // Now read the data back in.
   scoped_refptr<OSExchangeData> data2(new OSExchangeData(data.get()));
   BookmarkDragData read_data;
   EXPECT_TRUE(read_data.Read(*data2));
   EXPECT_EQ(g12->GetTitle(), read_data.title);
-  EXPECT_EQ(profile_id, read_data.profile_id);
   EXPECT_TRUE(read_data.is_valid);
   EXPECT_FALSE(read_data.is_url);
 
-  BookmarkNode* r_g12 = read_data.GetNode(&model);
+  BookmarkNode* r_g12 = read_data.GetNode(&profile);
   EXPECT_TRUE(g12 == r_g12);
+
+  TestingProfile profile2(1);
+  EXPECT_TRUE(read_data.GetNode(&profile2) == NULL);
 }
 
 TEST_F(BookmarkDragDataTest, GroupWithChild) {
-  BookmarkModel model(NULL);
-  BookmarkNode* root = model.GetBookmarkBarNode();
-  BookmarkNode* group = model.AddGroup(root, 0, L"g1");
+  TestingProfile profile;
+  profile.SetID(L"id");
+  profile.CreateBookmarkModel(false);
+  BookmarkModel* model = profile.GetBookmarkModel();
+  BookmarkNode* root = model->GetBookmarkBarNode();
+  BookmarkNode* group = model->AddGroup(root, 0, L"g1");
 
   GURL url(GURL("http://foo.com"));
-  const std::wstring profile_id(L"blah");
   const std::wstring title(L"blah2");
 
-  model.AddURL(group, 0, title, url);
+  model->AddURL(group, 0, title, url);
 
   BookmarkDragData drag_data(group);
-  drag_data.profile_id = profile_id;
 
   scoped_refptr<OSExchangeData> data(new OSExchangeData());
-  drag_data.Write(data.get());
+  drag_data.Write(&profile, data.get());
 
   // Now read the data back in.
   scoped_refptr<OSExchangeData> data2(new OSExchangeData(data.get()));
@@ -115,6 +125,6 @@ TEST_F(BookmarkDragDataTest, GroupWithChild) {
   EXPECT_TRUE(url == read_data.children[0].url);
   EXPECT_TRUE(read_data.children[0].is_url);
 
-  BookmarkNode* r_group = read_data.GetNode(&model);
+  BookmarkNode* r_group = read_data.GetNode(&profile);
   EXPECT_TRUE(group == r_group);
 }
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc
index af5a43f..0b7a212 100644
--- a/chrome/browser/bookmarks/bookmark_model.cc
+++ b/chrome/browser/bookmarks/bookmark_model.cc
@@ -265,21 +265,47 @@ void BookmarkModel::SetTitle(BookmarkNode* node,
                     BookmarkNodeChanged(this, node));
 }
 
-BookmarkNode* BookmarkModel::GetNodeByURL(const GURL& url) {
+void BookmarkModel::GetNodesByURL(const GURL& url,
+                                  std::vector<BookmarkNode*>* nodes) {
   AutoLock url_lock(url_lock_);
   BookmarkNode tmp_node(this, url);
   NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find(&tmp_node);
-  return (i != nodes_ordered_by_url_set_.end()) ? *i : NULL;
+  while (i != nodes_ordered_by_url_set_.end() && (*i)->GetURL() == url) {
+    nodes->push_back(*i);
+    ++i;
+  }
+}
+
+BookmarkNode* BookmarkModel::GetMostRecentlyAddedNodeForURL(const GURL& url) {
+  std::vector<BookmarkNode*> nodes;
+  GetNodesByURL(url, &nodes);
+  if (nodes.empty())
+    return NULL;
+
+  std::sort(nodes.begin(), nodes.end(), &MoreRecentlyAdded);
+  return nodes.front();
 }
 
 void BookmarkModel::GetBookmarks(std::vector<GURL>* urls) {
   AutoLock url_lock(url_lock_);
+  const GURL* last_url = NULL;
   for (NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.begin();
        i != nodes_ordered_by_url_set_.end(); ++i) {
-    urls->push_back((*i)->GetURL());
+    const GURL* url = &((*i)->url_);
+    // Only add unique URLs.
+    if (!last_url || *url != *last_url)
+      urls->push_back(*url);
+    last_url = url;
   }
 }
 
+bool BookmarkModel::IsBookmarked(const GURL& url) {
+  AutoLock url_lock(url_lock_);
+  BookmarkNode tmp_node(this, url);
+  return (nodes_ordered_by_url_set_.find(&tmp_node) !=
+          nodes_ordered_by_url_set_.end());
+}
+
 BookmarkNode* BookmarkModel::GetNodeByID(int id) {
   // TODO(sky): TreeNode needs a method that visits all nodes using a predicate.
   return GetNodeByID(&root_, id);
@@ -299,7 +325,7 @@ BookmarkNode* BookmarkModel::AddGroup(
   new_node->SetTitle(title);
   new_node->type_ = history::StarredEntry::USER_GROUP;
 
-  return AddNode(parent, index, new_node);
+  return AddNode(parent, index, new_node, false);
 }
 
 BookmarkNode* BookmarkModel::AddURL(BookmarkNode* parent,
@@ -321,12 +347,7 @@ BookmarkNode* BookmarkModel::AddURLWithCreationTime(
     return NULL;
   }
 
-  BookmarkNode* existing_node = GetNodeByURL(url);
-  if (existing_node) {
-    Move(existing_node, parent, index);
-    SetTitle(existing_node, title);
-    return existing_node;
-  }
+  bool was_bookmarked = IsBookmarked(url);
 
   SetDateGroupModified(parent, creation_time);
 
@@ -338,19 +359,28 @@ BookmarkNode* BookmarkModel::AddURLWithCreationTime(
   AutoLock url_lock(url_lock_);
   nodes_ordered_by_url_set_.insert(new_node);
 
-  return AddNode(parent, index, new_node);
+  return AddNode(parent, index, new_node, was_bookmarked);
 }
 
 void BookmarkModel::SetURLStarred(const GURL& url,
                                   const std::wstring& title,
                                   bool is_starred) {
-  BookmarkNode* node = GetNodeByURL(url);
-  if (is_starred && !node) {
-    // Add the url.
+  std::vector<BookmarkNode*> bookmarks;
+  GetNodesByURL(url, &bookmarks);
+  bool bookmarks_exist = !bookmarks.empty();
+  if (is_starred == bookmarks_exist)
+    return;  // Nothing to do, state already matches.
+
+  if (is_starred) {
+    // Create a bookmark.
     BookmarkNode* parent = GetParentForNewNodes();
     AddURL(parent, parent->GetChildCount(), title, url);
-  } else if (!is_starred && node) {
-    Remove(node->GetParent(), node->GetParent()->IndexOfChild(node));
+  } else {
+    // Remove all the bookmarks.
+    for (size_t i = 0; i < bookmarks.size(); ++i) {
+      BookmarkNode* node = bookmarks[i];
+      Remove(node->GetParent(), node->GetParent()->IndexOfChild(node));
+    }
   }
 }
 
@@ -377,6 +407,10 @@ void BookmarkModel::RemoveNode(BookmarkNode* node,
     // 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);
     removed_urls->insert(node->GetURL());
   }
@@ -480,6 +514,16 @@ void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
   {
     AutoLock url_lock(url_lock_);
     RemoveNode(node.get(), &details.changed_urls);
+
+    // RemoveNode adds an entry to changed_urls for each node of type URL. As we
+    // allow duplicates we need to remove any entries that are still bookmarked.
+    for (std::set<GURL>::iterator i = details.changed_urls.begin();
+         i != details.changed_urls.end(); ){
+      if (IsBookmarked(*i))
+        i = details.changed_urls.erase(i);
+      else
+        ++i;
+    }
   }
 
   if (store_.get())
@@ -488,6 +532,11 @@ void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeRemoved(this, parent, index));
 
+  if (details.changed_urls.empty()) {
+    // No point in sending out notification if the starred state didn't change.
+    return;
+  }
+
   if (profile_) {
     HistoryService* history =
         profile_->GetHistoryService(Profile::EXPLICIT_ACCESS);
@@ -502,7 +551,8 @@ void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) {
 
 BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
                                      int index,
-                                     BookmarkNode* node) {
+                                     BookmarkNode* node,
+                                     bool was_bookmarked) {
   parent->Add(index, node);
 
   if (store_.get())
@@ -511,7 +561,7 @@ BookmarkNode* BookmarkModel::AddNode(BookmarkNode* parent,
   FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
                     BookmarkNodeAdded(this, parent, index));
 
-  if (node->GetType() == history::StarredEntry::URL) {
+  if (node->GetType() == history::StarredEntry::URL && !was_bookmarked) {
     history::URLsStarredDetails details(true);
     details.changed_urls.insert(node->GetURL());
     NotificationService::current()->Notify(NOTIFY_URLS_STARRED,
@@ -662,9 +712,11 @@ void BookmarkModel::Observe(NotificationType type,
       Details<history::FavIconChangeDetails> favicon_details(details);
       for (std::set<GURL>::const_iterator i = favicon_details->urls.begin();
            i != favicon_details->urls.end(); ++i) {
-        BookmarkNode* node = GetNodeByURL(*i);
-        if (node) {
+        std::vector<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 = nodes[i];
           node->InvalidateFavicon();
           CancelPendingFavIconLoadRequests(node);
           FOR_EACH_OBSERVER(BookmarkModelObserver, observers_,
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index e20349d..a9f6308 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -18,6 +18,7 @@
 #include "googleurl/src/gurl.h"
 #include "skia/include/SkBitmap.h"
 
+class BookmarkEditorView;
 class BookmarkModel;
 class BookmarkCodec;
 class Profile;
@@ -32,10 +33,14 @@ class StarredURLDatabase;
 // star id and type. BookmarkNodes are returned from a BookmarkModel.
 //
 class BookmarkNode : public ChromeViews::TreeNode<BookmarkNode> {
+  friend class BookmarkEditorView;
   friend class BookmarkModel;
   friend class BookmarkCodec;
   friend class history::StarredURLDatabase;
+  FRIEND_TEST(BookmarkEditorViewTest, ChangeParentAndURL);
+  FRIEND_TEST(BookmarkEditorViewTest, EditURLKeepsPosition);
   FRIEND_TEST(BookmarkModelTest, MostRecentlyAddedEntries);
+  FRIEND_TEST(BookmarkModelTest, GetMostRecentlyAddedNodeForURL);
 
  public:
   BookmarkNode(BookmarkModel* model, const GURL& url);
@@ -114,7 +119,7 @@ class BookmarkNode : public ChromeViews::TreeNode<BookmarkNode> {
   // Time last modified. Only used for groups.
   Time date_group_modified_;
 
-  DISALLOW_EVIL_CONSTRUCTORS(BookmarkNode);
+  DISALLOW_COPY_AND_ASSIGN(BookmarkNode);
 };
 
 // BookmarkModelObserver ------------------------------------------------------
@@ -161,15 +166,11 @@ class BookmarkModelObserver {
 // and groups. Two graphs are provided for the two entry points: those on
 // the bookmark bar, and those in the other folder.
 //
-// The methods of BookmarkModel update the internal structure immediately
-// and update the backend in the background.
-//
 // An observer may be attached to observer relevant events.
 //
 // You should NOT directly create a BookmarkModel, instead go through the
 // Profile.
 
-// TODO(sky): rename to BookmarkModel.
 class BookmarkModel : public NotificationObserver, public BookmarkService {
   friend class BookmarkNode;
   friend class BookmarkModelTest;
@@ -241,18 +242,19 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // Returns true if the model finished loading.
   bool IsLoaded() { return loaded_; }
 
-  // Returns the node with the specified URL, or NULL if there is no node with
-  // the specified URL. This method is thread safe.
-  BookmarkNode* GetNodeByURL(const GURL& url);
+  // Returns the set of nodes with the specified URL.
+  void GetNodesByURL(const GURL& url, std::vector<BookmarkNode*>* nodes);
+
+  // Returns the most recently added node for the url. Returns NULL if url is
+  // not bookmarked.
+  BookmarkNode* GetMostRecentlyAddedNodeForURL(const GURL& url);
 
   // Returns all the bookmarked urls. This method is thread safe.
   virtual void GetBookmarks(std::vector<GURL>* urls);
 
   // Returns true if there is a bookmark for the specified URL. This method is
   // thread safe. See BookmarkService for more details on this.
-  virtual bool IsBookmarked(const GURL& url) {
-    return GetNodeByURL(url) != NULL;
-  }
+  virtual bool IsBookmarked(const GURL& url);
 
   // Blocks until loaded; this is NOT invoked on the main thread. See
   // BookmarkService for more details on this.
@@ -267,8 +269,7 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
                          int index,
                          const std::wstring& title);
 
-  // Adds a url at the specified position. If there is already a node with the
-  // specified URL, it is moved to the new position.
+  // Adds a url at the specified position.
   BookmarkNode* AddURL(BookmarkNode* parent,
                        int index,
                        const std::wstring& title,
@@ -281,9 +282,9 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
                                        const GURL& url,
                                        const Time& creation_time);
 
-  // This is the convenience that makes sure the url is starred or not
-  // starred. If the URL is not currently starred, it is added to the
-  // most recent parent.
+  // This is the convenience that makes sure the url is starred or not starred.
+  // If is_starred is false, all bookmarks for URL are removed. If is_starred is
+  // true and there are no bookmarks for url, a bookmark is created.
   void SetURLStarred(const GURL& url,
                      const std::wstring& title,
                      bool is_starred);
@@ -305,7 +306,7 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // Overriden to notify the observer the favicon has been loaded.
   void FavIconLoaded(BookmarkNode* node);
 
-  // Removes the node from internal maps and recurces through all children. If
+  // Removes the node from internal maps and recurses through all children. If
   // the node is a url, its url is added to removed_urls.
   //
   // This does NOT delete the node.
@@ -339,10 +340,13 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // type specifies how the node should be removed.
   void RemoveAndDeleteNode(BookmarkNode* delete_me);
 
-  // Adds the node at the specified position, and sends notification.
+  // Adds the node at the specified position and sends notification. If
+  // was_bookmarked is true, it indicates a bookmark already existed for the
+  // URL.
   BookmarkNode* AddNode(BookmarkNode* parent,
                         int index,
-                        BookmarkNode* node);
+                        BookmarkNode* node,
+                        bool was_bookmarked);
 
   // Implementation of GetNodeByID.
   BookmarkNode* GetNodeByID(BookmarkNode* node, int id);
@@ -409,7 +413,7 @@ class BookmarkModel : public NotificationObserver, public BookmarkService {
   // urls.
   // WARNING: nodes_ordered_by_url_set_ is accessed on multiple threads. As
   // such, be sure and wrap all usage of it around url_lock_.
-  typedef std::set<BookmarkNode*,NodeURLComparator> NodesOrderedByURLSet;
+  typedef std::multiset<BookmarkNode*,NodeURLComparator> NodesOrderedByURLSet;
   NodesOrderedByURLSet nodes_ordered_by_url_set_;
   Lock url_lock_;
 
diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc
index 04c08d3..a6cab2a 100644
--- a/chrome/browser/bookmarks/bookmark_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc
@@ -7,6 +7,7 @@
 #include "chrome/browser/bookmarks/bookmark_codec.h"
 #include "chrome/common/chrome_constants.h"
 #include "chrome/common/chrome_paths.h"
+#include "chrome/common/notification_registrar.h"
 #include "chrome/test/testing_profile.h"
 #include "chrome/views/tree_node_model.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -169,7 +170,7 @@ TEST_F(BookmarkModelTest, AddURL) {
   ASSERT_EQ(title, new_node->GetTitle());
   ASSERT_TRUE(url == new_node->GetURL());
   ASSERT_EQ(history::StarredEntry::URL, new_node->GetType());
-  ASSERT_TRUE(new_node == model.GetNodeByURL(url));
+  ASSERT_TRUE(new_node == model.GetMostRecentlyAddedNodeForURL(url));
 
   EXPECT_TRUE(new_node->id() != root->id() &&
               new_node->id() != model.other_node()->id());
@@ -210,7 +211,7 @@ TEST_F(BookmarkModelTest, RemoveURL) {
   observer_details.AssertEquals(root, NULL, 0, -1);
 
   // Make sure there is no mapping for the URL.
-  ASSERT_TRUE(model.GetNodeByURL(url) == NULL);
+  ASSERT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == NULL);
 }
 
 TEST_F(BookmarkModelTest, RemoveGroup) {
@@ -233,7 +234,7 @@ TEST_F(BookmarkModelTest, RemoveGroup) {
   observer_details.AssertEquals(root, NULL, 0, -1);
 
   // Make sure there is no mapping for the URL.
-  ASSERT_TRUE(model.GetNodeByURL(url) == NULL);
+  ASSERT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == NULL);
 }
 
 TEST_F(BookmarkModelTest, SetTitle) {
@@ -274,7 +275,7 @@ TEST_F(BookmarkModelTest, Move) {
   model.Remove(root, 0);
   AssertObserverCount(0, 0, 1, 0);
   observer_details.AssertEquals(root, NULL, 0, -1);
-  EXPECT_TRUE(model.GetNodeByURL(url) == NULL);
+  EXPECT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == NULL);
   EXPECT_EQ(0, root->GetChildCount());
 }
 
@@ -369,6 +370,110 @@ TEST_F(BookmarkModelTest, GetBookmarksMatchingText) {
   EXPECT_EQ(n2, results[0].node);
 }
 
+// Makes sure GetMostRecentlyAddedNodeForURL stays in sync.
+TEST_F(BookmarkModelTest, GetMostRecentlyAddedNodeForURL) {
+  // Add a couple of nodes such that the following holds for the time of the
+  // nodes: n1 > n2
+  Time base_time = Time::Now();
+  const GURL url("http://foo.com/0");
+  BookmarkNode* n1 = model.AddURL(model.GetBookmarkBarNode(), 0, L"blah", url);
+  BookmarkNode* n2 = model.AddURL(model.GetBookmarkBarNode(), 1, L"blah", url);
+  n1->date_added_ = base_time + TimeDelta::FromDays(4);
+  n2->date_added_ = base_time + TimeDelta::FromDays(3);
+
+  // Make sure order is honored.
+  ASSERT_EQ(n1, model.GetMostRecentlyAddedNodeForURL(url));
+
+  // swap 1 and 2, then check again.
+  std::swap(n1->date_added_, n2->date_added_);
+  ASSERT_EQ(n2, model.GetMostRecentlyAddedNodeForURL(url));
+}
+
+// Makes sure GetBookmarks removes duplicates.
+TEST_F(BookmarkModelTest, GetBookmarksWithDups) {
+  const GURL url("http://foo.com/0");
+  model.AddURL(model.GetBookmarkBarNode(), 0, L"blah", url);
+  model.AddURL(model.GetBookmarkBarNode(), 1, L"blah", url);
+
+  std::vector<GURL> urls;
+  model.GetBookmarks(&urls);
+  EXPECT_EQ(1, urls.size());
+  ASSERT_TRUE(urls[0] == url);
+}
+
+namespace {
+
+// NotificationObserver implementation used in verifying we've received the
+// NOTIFY_URLS_STARRED method correctly.
+class StarredListener : public NotificationObserver {
+ public:
+  StarredListener() : notification_count_(0), details_(false) {
+    registrar_.Add(this, NOTIFY_URLS_STARRED, Source<Profile>(NULL));
+  }
+
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details) {
+    if (type == NOTIFY_URLS_STARRED) {
+      notification_count_++;
+      details_ = *(Details<history::URLsStarredDetails>(details).ptr());
+    }
+  }
+
+  // Number of times NOTIFY_URLS_STARRED has been observed.
+  int notification_count_;
+
+  // Details from the last NOTIFY_URLS_STARRED.
+  history::URLsStarredDetails details_;
+
+ private:
+  NotificationRegistrar registrar_;
+
+  DISALLOW_COPY_AND_ASSIGN(StarredListener);
+};
+
+}  // namespace
+
+// Makes sure NOTIFY_URLS_STARRED is sent correctly.
+TEST_F(BookmarkModelTest, NotifyURLsStarred) {
+  StarredListener listener;
+  const GURL url("http://foo.com/0");
+  BookmarkNode* n1 = model.AddURL(model.GetBookmarkBarNode(), 0, L"blah", url);
+
+  // Starred notification should be sent.
+  EXPECT_EQ(1, listener.notification_count_);
+  ASSERT_TRUE(listener.details_.starred);
+  ASSERT_EQ(1, listener.details_.changed_urls.size());
+  EXPECT_TRUE(url == *(listener.details_.changed_urls.begin()));
+  listener.notification_count_ = 0;
+  listener.details_.changed_urls.clear();
+
+  // Add another bookmark for the same URL. This should not send any
+  // notification.
+  BookmarkNode* n2 = model.AddURL(model.GetBookmarkBarNode(), 1, L"blah", url);
+
+  EXPECT_EQ(0, listener.notification_count_);
+
+  // Remove n2.
+  model.Remove(n2->GetParent(), 1);
+  n2 = NULL;
+
+  // Shouldn't have received any notification as n1 still exists with the same
+  // URL.
+  EXPECT_EQ(0, listener.notification_count_);
+
+  EXPECT_TRUE(model.GetMostRecentlyAddedNodeForURL(url) == n1);
+
+  // Remove n1.
+  model.Remove(n1->GetParent(), 0);
+
+  // Now we should get the notification.
+  EXPECT_EQ(1, listener.notification_count_);
+  ASSERT_FALSE(listener.details_.starred);
+  ASSERT_EQ(1, listener.details_.changed_urls.size());
+  EXPECT_TRUE(url == *(listener.details_.changed_urls.begin()));
+}
+
 namespace {
 
 // See comment in PopulateNodeFromString.
@@ -650,7 +755,7 @@ class BookmarkModelTestWithProfile2 : public BookmarkModelTestWithProfile {
     ASSERT_TRUE(child->GetURL() ==
                 GURL("http://www.google.com/intl/en/about.html"));
 
-    ASSERT_TRUE(bb_model_->GetNodeByURL(GURL("http://www.google.com")) != NULL);
+    ASSERT_TRUE(bb_model_->IsBookmarked(GURL("http://www.google.com")));
   }
 };
 
diff --git a/chrome/browser/bookmarks/bookmark_service.h b/chrome/browser/bookmarks/bookmark_service.h
index a758382..30387a3 100644
--- a/chrome/browser/bookmarks/bookmark_service.h
+++ b/chrome/browser/bookmarks/bookmark_service.h
@@ -48,7 +48,9 @@ class BookmarkService {
   // If not on the main thread you *must* invoke BlockTillLoaded first.
   virtual bool IsBookmarked(const GURL& url) = 0;
 
-  // Returns, by reference in urls, the set of bookmarked urls.
+  // Returns, by reference in urls, the set of bookmarked urls. This returns
+  // the unique set of URLs. For example, if two bookmarks reference the same
+  // URL only one entry is added.
   //
   // If not on the main thread you *must* invoke BlockTillLoaded first.
   virtual void GetBookmarks(std::vector<GURL>* urls) = 0;