summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authortom.cassiotis@gmail.com <tom.cassiotis@gmail.com@0039d316-1c4b-4281-b951-d872f2087c98>2013-06-10 16:49:18 +0000
committertom.cassiotis@gmail.com <tom.cassiotis@gmail.com@0039d316-1c4b-4281-b951-d872f2087c98>2013-06-10 16:49:18 +0000
commit472f95ed65083a54004c78d2dc519c1bb936fcb1 (patch)
treec588fa40fd2339059459996f2564dc7c761e2a17
parent39841f3451c1063363f2fee3cb32564b18ffb5a9 (diff)
downloadchromium_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.cc36
-rw-r--r--chrome/browser/bookmarks/bookmark_model.h7
-rw-r--r--chrome/browser/bookmarks/bookmark_model_observer.h21
-rw-r--r--chrome/browser/bookmarks/bookmark_model_unittest.cc97
-rw-r--r--chrome/browser/bookmarks/bookmark_node_data.h2
-rw-r--r--ui/base/models/tree_node_model.h8
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_; }