diff options
author | tom.cassiotis@gmail.com <tom.cassiotis@gmail.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-10 16:49:18 +0000 |
---|---|---|
committer | tom.cassiotis@gmail.com <tom.cassiotis@gmail.com@0039d316-1c4b-4281-b951-d872f2087c98> | 2013-06-10 16:49:18 +0000 |
commit | 472f95ed65083a54004c78d2dc519c1bb936fcb1 (patch) | |
tree | c588fa40fd2339059459996f2564dc7c761e2a17 | |
parent | 39841f3451c1063363f2fee3cb32564b18ffb5a9 (diff) | |
download | chromium_src-472f95ed65083a54004c78d2dc519c1bb936fcb1.zip chromium_src-472f95ed65083a54004c78d2dc519c1bb936fcb1.tar.gz chromium_src-472f95ed65083a54004c78d2dc519c1bb936fcb1.tar.bz2 |
BookmarkModel changes for multiple level undo and redo of bookmarks.
Staged commit of a larger feature to ease review and landing the change.
- New BookmarkModelObserver entries to be alerted before an action is
executed.
- BookmarkModel ability to explicitly reorder all children of a bookmark
tree. Support function also added to TreeNode for this operation.
- Test updated BookmarkModel and BookmarkModelObserver changes.
BUG=126092
Review URL: https://chromiumcodereview.appspot.com/16479003
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@205228 0039d316-1c4b-4281-b951-d872f2087c98
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model.cc | 36 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model.h | 7 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model_observer.h | 21 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_model_unittest.cc | 97 | ||||
-rw-r--r-- | chrome/browser/bookmarks/bookmark_node_data.h | 2 | ||||
-rw-r--r-- | ui/base/models/tree_node_model.h | 8 |
6 files changed, 153 insertions, 18 deletions
diff --git a/chrome/browser/bookmarks/bookmark_model.cc b/chrome/browser/bookmarks/bookmark_model.cc index 441e3f3..03caa79 100644 --- a/chrome/browser/bookmarks/bookmark_model.cc +++ b/chrome/browser/bookmarks/bookmark_model.cc @@ -299,6 +299,10 @@ void BookmarkModel::Remove(const BookmarkNode* parent, int index) { void BookmarkModel::RemoveAll() { std::set<GURL> changed_urls; ScopedVector<BookmarkNode> removed_nodes; + + FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, + OnWillRemoveAllBookmarks(this)); + BeginExtensiveChanges(); // Skip deleting permanent nodes. Permanent bookmark nodes are the root and // its immediate children. For removing all non permanent nodes just remove @@ -412,6 +416,9 @@ void BookmarkModel::SetTitle(const BookmarkNode* node, const string16& title) { return; } + FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, + OnWillChangeBookmarkNode(this, node)); + // The title index doesn't support changing the title, instead we remove then // add it back. index_->Remove(node); @@ -444,6 +451,9 @@ void BookmarkModel::SetURL(const BookmarkNode* node, const GURL& url) { mutable_node->InvalidateFavicon(); CancelPendingFaviconLoadRequests(mutable_node); + FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, + OnWillChangeBookmarkNode(this, node)); + { base::AutoLock url_lock(url_lock_); NodesOrderedByURLSet::iterator i = nodes_ordered_by_url_set_.find( @@ -627,6 +637,9 @@ void BookmarkModel::SortChildren(const BookmarkNode* parent) { return; } + FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, + OnWillReorderBookmarkNode(this, parent)); + UErrorCode error = U_ZERO_ERROR; scoped_ptr<icu::Collator> collator(icu::Collator::createInstance(error)); if (U_FAILURE(error)) @@ -643,6 +656,26 @@ void BookmarkModel::SortChildren(const BookmarkNode* parent) { BookmarkNodeChildrenReordered(this, parent)); } +void BookmarkModel::ReorderChildren( + const BookmarkNode* parent, + const std::vector<BookmarkNode*>& ordered_nodes) { + // Ensure that all children in |parent| are in |ordered_nodes|. + DCHECK_EQ(static_cast<size_t>(parent->child_count()), ordered_nodes.size()); + for (size_t i = 0; i < ordered_nodes.size(); ++i) + DCHECK_EQ(parent, ordered_nodes[i]->parent()); + + FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, + OnWillReorderBookmarkNode(this, parent)); + + AsMutable(parent)->SetChildren(ordered_nodes); + + if (store_.get()) + store_->ScheduleSave(); + + FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, + BookmarkNodeChildrenReordered(this, parent)); +} + void BookmarkModel::SetDateFolderModified(const BookmarkNode* parent, const Time time) { DCHECK(parent); @@ -779,6 +812,9 @@ void BookmarkModel::RemoveAndDeleteNode(BookmarkNode* delete_me) { DCHECK(parent); int index = parent->GetIndexOf(node.get()); + FOR_EACH_OBSERVER(BookmarkModelObserver, observers_, + OnWillRemoveBookmarks(this, parent, index, node.get())); + std::set<GURL> changed_urls; { base::AutoLock url_lock(url_lock_); diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h index f55b8be..130e5a5 100644 --- a/chrome/browser/bookmarks/bookmark_model.h +++ b/chrome/browser/bookmarks/bookmark_model.h @@ -369,6 +369,13 @@ class BookmarkModel : public content::NotificationObserver, // BookmarkNodeChildrenReordered method. void SortChildren(const BookmarkNode* parent); + // Order the children of |parent| as specified in |ordered_nodes|. This + // function should only be used to reorder the child nodes of |parent| and + // is not meant to move nodes between different parent. Notifies observers + // using the BookmarkNodeChildrenReordered method. + void ReorderChildren(const BookmarkNode* parent, + const std::vector<BookmarkNode*>& ordered_nodes); + // Sets the date when the folder was modified. void SetDateFolderModified(const BookmarkNode* node, const base::Time time); diff --git a/chrome/browser/bookmarks/bookmark_model_observer.h b/chrome/browser/bookmarks/bookmark_model_observer.h index 9a759c2..31bd2e7 100644 --- a/chrome/browser/bookmarks/bookmark_model_observer.h +++ b/chrome/browser/bookmarks/bookmark_model_observer.h @@ -30,6 +30,15 @@ class BookmarkModelObserver { const BookmarkNode* parent, int index) = 0; + // Invoked before a node is removed. + // |parent| the parent of the node that will be removed. + // |old_index| the index of the node about to be removed in |parent|. + // |node| is the node to be removed. + virtual void OnWillRemoveBookmarks(BookmarkModel* model, + const BookmarkNode* parent, + int old_index, + const BookmarkNode* node) {} + // Invoked when a node has been removed, the item may still be starred though. // |parent| the parent of the node that was removed. // |old_index| the index of the removed node in |parent| before it was @@ -40,6 +49,10 @@ class BookmarkModelObserver { int old_index, const BookmarkNode* node) = 0; + // Invoked before the title or url of a node is changed. + virtual void OnWillChangeBookmarkNode(BookmarkModel* model, + const BookmarkNode* node) {} + // Invoked when the title or url of a node changes. virtual void BookmarkNodeChanged(BookmarkModel* model, const BookmarkNode* node) = 0; @@ -48,6 +61,11 @@ class BookmarkModelObserver { virtual void BookmarkNodeFaviconChanged(BookmarkModel* model, const BookmarkNode* node) = 0; + // Invoked before the direct children of |node| have been reordered in some + // way, such as sorted. + virtual void OnWillReorderBookmarkNode(BookmarkModel* model, + const BookmarkNode* node) {} + // Invoked when the children (just direct children, not descendants) of // |node| have been reordered in some way, such as sorted. virtual void BookmarkNodeChildrenReordered(BookmarkModel* model, @@ -66,6 +84,9 @@ class BookmarkModelObserver { // update to finish. virtual void ExtensiveBookmarkChangesEnded(BookmarkModel* model) {} + // Invoked before all non-permanent bookmark nodes are removed. + virtual void OnWillRemoveAllBookmarks(BookmarkModel* model) {} + // Invoked when all non-permanent bookmark nodes have been removed. virtual void BookmarkAllNodesRemoved(BookmarkModel* model) = 0; diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc index 000712f..2ffb2b6 100644 --- a/chrome/browser/bookmarks/bookmark_model_unittest.cc +++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc @@ -168,6 +168,13 @@ class BookmarkModelTest : public testing::Test, observer_details_.Set(parent, NULL, index, -1); } + virtual void OnWillRemoveBookmarks(BookmarkModel* model, + const BookmarkNode* parent, + int old_index, + const BookmarkNode* node) OVERRIDE { + ++before_remove_count_; + } + virtual void BookmarkNodeRemoved(BookmarkModel* model, const BookmarkNode* parent, int old_index, @@ -182,12 +189,22 @@ class BookmarkModelTest : public testing::Test, observer_details_.Set(node, NULL, -1, -1); } + virtual void OnWillChangeBookmarkNode(BookmarkModel* model, + const BookmarkNode* node) OVERRIDE { + ++before_change_count_; + } + virtual void BookmarkNodeChildrenReordered( BookmarkModel* model, const BookmarkNode* node) OVERRIDE { ++reordered_count_; } + virtual void OnWillReorderBookmarkNode(BookmarkModel* model, + const BookmarkNode* node) OVERRIDE { + ++before_reorder_count_; + } + virtual void BookmarkNodeFaviconChanged(BookmarkModel* model, const BookmarkNode* node) OVERRIDE { // We never attempt to load favicons, so that this method never @@ -207,22 +224,36 @@ class BookmarkModelTest : public testing::Test, ++all_bookmarks_removed_; } + virtual void OnWillRemoveAllBookmarks(BookmarkModel* model) OVERRIDE { + ++before_remove_all_count_; + } + void ClearCounts() { added_count_ = moved_count_ = removed_count_ = changed_count_ = reordered_count_ = extensive_changes_beginning_count_ = - extensive_changes_ended_count_ = all_bookmarks_removed_ = 0; + extensive_changes_ended_count_ = all_bookmarks_removed_ = + before_remove_count_ = before_change_count_ = before_reorder_count_ = + before_remove_all_count_ = 0; } void AssertObserverCount(int added_count, int moved_count, int removed_count, int changed_count, - int reordered_count) { + int reordered_count, + int before_remove_count, + int before_change_count, + int before_reorder_count, + int before_remove_all_count) { EXPECT_EQ(added_count_, added_count); EXPECT_EQ(moved_count_, moved_count); EXPECT_EQ(removed_count_, removed_count); EXPECT_EQ(changed_count_, changed_count); EXPECT_EQ(reordered_count_, reordered_count); + EXPECT_EQ(before_remove_count_, before_remove_count); + EXPECT_EQ(before_change_count_, before_change_count); + EXPECT_EQ(before_reorder_count_, before_reorder_count); + EXPECT_EQ(before_remove_all_count_, before_remove_all_count); } void AssertExtensiveChangesObserverCount( @@ -248,6 +279,10 @@ class BookmarkModelTest : public testing::Test, int extensive_changes_beginning_count_; int extensive_changes_ended_count_; int all_bookmarks_removed_; + int before_remove_count_; + int before_change_count_; + int before_reorder_count_; + int before_remove_all_count_; DISALLOW_COPY_AND_ASSIGN(BookmarkModelTest); }; @@ -279,7 +314,7 @@ TEST_F(BookmarkModelTest, AddURL) { const GURL url("http://foo.com"); const BookmarkNode* new_node = model_.AddURL(root, 0, title, url); - AssertObserverCount(1, 0, 0, 0, 0); + AssertObserverCount(1, 0, 0, 0, 0, 0, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); ASSERT_EQ(1, root->child_count()); @@ -300,7 +335,7 @@ TEST_F(BookmarkModelTest, AddURLWithUnicodeTitle) { const GURL url("https://www.baidu.com/"); const BookmarkNode* new_node = model_.AddURL(root, 0, title, url); - AssertObserverCount(1, 0, 0, 0, 0); + AssertObserverCount(1, 0, 0, 0, 0, 0, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); ASSERT_EQ(1, root->child_count()); @@ -337,7 +372,7 @@ TEST_F(BookmarkModelTest, AddURLToMobileBookmarks) { const GURL url("http://foo.com"); const BookmarkNode* new_node = model_.AddURL(root, 0, title, url); - AssertObserverCount(1, 0, 0, 0, 0); + AssertObserverCount(1, 0, 0, 0, 0, 0, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); ASSERT_EQ(1, root->child_count()); @@ -356,7 +391,7 @@ TEST_F(BookmarkModelTest, AddFolder) { const string16 title(ASCIIToUTF16("foo")); const BookmarkNode* new_node = model_.AddFolder(root, 0, title); - AssertObserverCount(1, 0, 0, 0, 0); + AssertObserverCount(1, 0, 0, 0, 0, 0, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); ASSERT_EQ(1, root->child_count()); @@ -370,7 +405,7 @@ TEST_F(BookmarkModelTest, AddFolder) { // Add another folder, just to make sure folder_ids are incremented correctly. ClearCounts(); model_.AddFolder(root, 0, title); - AssertObserverCount(1, 0, 0, 0, 0); + AssertObserverCount(1, 0, 0, 0, 0, 0, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); } @@ -399,7 +434,7 @@ TEST_F(BookmarkModelTest, RemoveURL) { model_.Remove(root, 0); ASSERT_EQ(0, root->child_count()); - AssertObserverCount(0, 0, 1, 0, 0); + AssertObserverCount(0, 0, 1, 0, 0, 1, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); // Make sure there is no mapping for the URL. @@ -422,7 +457,7 @@ TEST_F(BookmarkModelTest, RemoveFolder) { // Now remove the folder. model_.Remove(root, 0); ASSERT_EQ(0, root->child_count()); - AssertObserverCount(0, 0, 1, 0, 0); + AssertObserverCount(0, 0, 1, 0, 0, 1, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); // Make sure there is no mapping for the URL. @@ -443,7 +478,7 @@ TEST_F(BookmarkModelTest, RemoveAll) { const BookmarkNode* folder = model_.AddFolder(bookmark_bar_node, 0, title); model_.AddURL(folder, 0, title, url); - AssertObserverCount(3, 0, 0, 0, 0); + AssertObserverCount(3, 0, 0, 0, 0, 0, 0, 0, 0); ClearCounts(); model_.RemoveAll(); @@ -451,7 +486,7 @@ TEST_F(BookmarkModelTest, RemoveAll) { EXPECT_EQ(0, bookmark_bar_node->child_count()); // No individual BookmarkNodeRemoved events are fired, so removed count // should be 0. - AssertObserverCount(0, 0, 0, 0, 0); + AssertObserverCount(0, 0, 0, 0, 0, 0, 0, 0, 1); AssertExtensiveChangesObserverCount(1, 1); EXPECT_EQ(1, AllNodesRemovedObserverCount()); } @@ -466,7 +501,7 @@ TEST_F(BookmarkModelTest, SetTitle) { title = ASCIIToUTF16("foo2"); model_.SetTitle(node, title); - AssertObserverCount(0, 0, 0, 1, 0); + AssertObserverCount(0, 0, 0, 1, 0, 0, 1, 0, 0); observer_details_.ExpectEquals(node, NULL, -1, -1); EXPECT_EQ(title, node->GetTitle()); } @@ -495,7 +530,7 @@ TEST_F(BookmarkModelTest, SetURL) { url = GURL("http://foo2.com"); model_.SetURL(node, url); - AssertObserverCount(0, 0, 0, 1, 0); + AssertObserverCount(0, 0, 0, 1, 0, 0, 1, 0, 0); observer_details_.ExpectEquals(node, NULL, -1, -1); EXPECT_EQ(url, node->url()); } @@ -510,7 +545,7 @@ TEST_F(BookmarkModelTest, SetDateAdded) { base::Time new_time = base::Time::Now() + base::TimeDelta::FromMinutes(20); model_.SetDateAdded(node, new_time); - AssertObserverCount(0, 0, 0, 0, 0); + AssertObserverCount(0, 0, 0, 0, 0, 0, 0, 0, 0); EXPECT_EQ(new_time, node->date_added()); EXPECT_EQ(new_time, model_.bookmark_bar_node()->date_folder_modified()); } @@ -525,7 +560,7 @@ TEST_F(BookmarkModelTest, Move) { model_.Move(node, folder1, 0); - AssertObserverCount(0, 1, 0, 0, 0); + AssertObserverCount(0, 1, 0, 0, 0, 0, 0, 0, 0); observer_details_.ExpectEquals(root, folder1, 1, 0); EXPECT_TRUE(folder1 == node->parent()); EXPECT_EQ(1, root->child_count()); @@ -536,7 +571,7 @@ TEST_F(BookmarkModelTest, Move) { // And remove the folder. ClearCounts(); model_.Remove(root, 0); - AssertObserverCount(0, 0, 1, 0, 0); + AssertObserverCount(0, 0, 1, 0, 0, 1, 0, 0, 0); observer_details_.ExpectEquals(root, NULL, 0, -1); EXPECT_TRUE(model_.GetMostRecentlyAddedNodeForURL(url) == NULL); EXPECT_EQ(0, root->child_count()); @@ -968,7 +1003,7 @@ TEST_F(BookmarkModelTest, Sort) { model_.SortChildren(parent); // Make sure we were notified. - AssertObserverCount(0, 0, 0, 0, 1); + AssertObserverCount(0, 0, 0, 0, 1, 0, 0, 1, 0); // Make sure the order matches (remember, 'a' and 'C' are folders and // come first). @@ -978,6 +1013,34 @@ TEST_F(BookmarkModelTest, Sort) { EXPECT_EQ(parent->GetChild(3)->GetTitle(), ASCIIToUTF16("d")); } +TEST_F(BookmarkModelTest, Reorder) { + // Populate the bookmark bar node with nodes 'A', 'B', 'C' and 'D'. + TestNode bbn; + PopulateNodeFromString("A B C D", &bbn); + BookmarkNode* parent = AsMutable(model_.bookmark_bar_node()); + PopulateBookmarkNode(&bbn, &model_, parent); + + ClearCounts(); + + // Reorder bar node's bookmarks in reverse order. + std::vector<BookmarkNode*> new_order; + new_order.push_back(parent->GetChild(3)); + new_order.push_back(parent->GetChild(2)); + new_order.push_back(parent->GetChild(1)); + new_order.push_back(parent->GetChild(0)); + model_.ReorderChildren(parent, new_order); + + // Make sure we were notified. + AssertObserverCount(0, 0, 0, 0, 1, 0, 0, 1, 0); + + // Make sure the order matches is correct (it should be reversed). + ASSERT_EQ(4, parent->child_count()); + EXPECT_EQ("D", UTF16ToASCII(parent->GetChild(0)->GetTitle())); + EXPECT_EQ("C", UTF16ToASCII(parent->GetChild(1)->GetTitle())); + EXPECT_EQ("B", UTF16ToASCII(parent->GetChild(2)->GetTitle())); + EXPECT_EQ("A", UTF16ToASCII(parent->GetChild(3)->GetTitle())); +} + TEST_F(BookmarkModelTest, NodeVisibility) { EXPECT_TRUE(model_.bookmark_bar_node()->IsVisible()); EXPECT_TRUE(model_.other_node()->IsVisible()); diff --git a/chrome/browser/bookmarks/bookmark_node_data.h b/chrome/browser/bookmarks/bookmark_node_data.h index 099ad90..f7b75db 100644 --- a/chrome/browser/bookmarks/bookmark_node_data.h +++ b/chrome/browser/bookmarks/bookmark_node_data.h @@ -57,7 +57,7 @@ struct BookmarkNodeData { // Children, only used for non-URL nodes. std::vector<Element> children; - int64 id() { return id_; } + int64 id() const { return id_; } private: friend struct BookmarkNodeData; diff --git a/ui/base/models/tree_node_model.h b/ui/base/models/tree_node_model.h index 5bfe46e..0ab6ff8 100644 --- a/ui/base/models/tree_node_model.h +++ b/ui/base/models/tree_node_model.h @@ -101,6 +101,14 @@ class TreeNode : public TreeModelNode { children_.weak_clear(); } + // Removes all existing children without deleting the nodes and adds all nodes + // contained in |children| into this node as children. + void SetChildren(const std::vector<NodeType*>& children) { + RemoveAll(); + for (size_t i = 0; i < children.size(); ++i) + Add(children[i], i); + } + // Returns the parent node, or NULL if this is the root node. const NodeType* parent() const { return parent_; } NodeType* parent() { return parent_; } |