Permanent folders changes

This cl removes the non visible bookmark folders from the recently modified folder list. When sync is turned off some folders may become invisible. If this non visible folder was the last one used to create a bookmark you may end up in the situation where you can bookmark in a folder you can't see. This avoid the situation.

This cl also changes the way BookmarkNode::IsVisible() is implemented. A subsequent cl will introduce new features in there but in order to do so the Bookmark node needs to have access to the state of the sync, itself stored in the profile. I've tried a couple of approaches before settling down on this one:

- Passing the profile as argument to IsVisible().
- moving IsVisible as BookmarkModel::IsNodeVisible(BookmarkNode*) as the BookmarkModel has a profile_ instance variable.

Both those approaches required to change the calls to IsVisible all over the place and there are quite a bit of them, mostly in platform specific code. I've tried them, but they end up creating very big CL with changes in most of the platform code, with code made ugly because there are more arguments everywhere.

The simpler solution I've settled on introduces a small subclass of BookmarkNode, used only for the 3 permanent nodes ("Bookmark Bar", "Synced" and "Other"). That class get a profile on creation, and can then implement the visibility required by the feature in development.

This cl also includes a small improvement in the way the order of permanent folders is encoded. Two places in the code where enforcing the same order. I made the order of the permanent folders in the child array of the bookmark root the canonical order. The other place now explicitly uses this order.


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

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

2 files changed, 75 insertions, 5 deletions

diff --git a/ui/base/models/tree_node_iterator.h b/ui/base/models/tree_node_iterator.h
index 03e68ab..5a05df9 100644
--- a/ui/base/models/tree_node_iterator.h
+++ b/ui/base/models/tree_node_iterator.h
@@ -23,7 +23,27 @@ namespace ui {
 template <class NodeType>
 class TreeNodeIterator {
  public:
-  explicit TreeNodeIterator(NodeType* node) {
+  // This contructor accepts an optional filter function |prune| which could be
+  // used to prune complete branches of the tree. The filter function will be
+  // evaluated on each tree node and if it evaluates to true the node and all
+  // its descendants will be skipped by the iterator.
+  TreeNodeIterator(NodeType* node, bool (*prune)(NodeType*))
+      : prune_(prune) {
+    int index = 0;
+
+    // Move forward through the children list until the first non prunable node.
+    // This is to satisfy the iterator invariant that the current index in the
+    // Position at the top of the _positions list must point to a node the
+    // iterator will be returning.
+    for (; index < node->child_count(); ++index)
+      if (!prune || !prune(node->GetChild(index)))
+        break;
+
+    if (index < node->child_count())
+      positions_.push(Position<NodeType>(node, index));
+  }
+
+  explicit TreeNodeIterator(NodeType* node) : prune_(NULL) {
     if (!node->empty())
       positions_.push(Position<NodeType>(node, 0));
   }
@@ -38,6 +58,7 @@ class TreeNodeIterator {
       return NULL;
     }
 
+    // There must always be a valid node in the current Position index.
     NodeType* result = positions_.top().node->GetChild(positions_.top().index);
 
     // Make sure we don't attempt to visit result again.
@@ -46,10 +67,18 @@ class TreeNodeIterator {
     // Iterate over result's children.
     positions_.push(Position<NodeType>(result, 0));
 
-    // Advance to next position.
-    while (!positions_.empty() && positions_.top().index >=
-           positions_.top().node->child_count()) {
-      positions_.pop();
+    // Advance to next valid node by skipping over the pruned nodes and the
+    // empty Positions. At the end of this loop two cases are possible:
+    // - the current index of the top() Position points to a valid node
+    // - the _position list is empty, the iterator has_next() will return false.
+    while (!positions_.empty()) {
+      if (positions_.top().index >= positions_.top().node->child_count())
+        positions_.pop(); // This Position is all processed, move to the next.
+      else if (prune_ &&
+          prune_(positions_.top().node->GetChild(positions_.top().index)))
+        positions_.top().index++;  // Prune the branch.
+      else
+        break;  // Now positioned at the next node to be returned.
     }
 
     return result;
@@ -66,6 +95,7 @@ class TreeNodeIterator {
   };
 
   std::stack<Position<NodeType> > positions_;
+  bool (*prune_)(NodeType*);
 
   DISALLOW_COPY_AND_ASSIGN(TreeNodeIterator);
 };
diff --git a/ui/base/models/tree_node_iterator_unittest.cc b/ui/base/models/tree_node_iterator_unittest.cc
index 13140e9..0aefe4d 100644
--- a/ui/base/models/tree_node_iterator_unittest.cc
+++ b/ui/base/models/tree_node_iterator_unittest.cc
@@ -38,4 +38,44 @@ TEST(TreeNodeIteratorTest, Test) {
   ASSERT_FALSE(iterator.has_next());
 }
 
+static bool PruneOdd(TreeNodeWithValue<int>* node) {
+  return node->value % 2;
+}
+
+static bool PruneEven(TreeNodeWithValue<int>* node) {
+  return !(node->value % 2);
+}
+
+// The tree used for testing:
+// * + 1
+//   + 2
+//   + 3 + 4 + 5
+//       + 7
+
+TEST(TreeNodeIteratorPruneTest, Test) {
+  TreeNodeWithValue<int> root;
+  root.Add(new TreeNodeWithValue<int>(1), 0);
+  root.Add(new TreeNodeWithValue<int>(2), 1);
+  TreeNodeWithValue<int>* f3 = new TreeNodeWithValue<int>(3);
+  root.Add(f3, 2);
+  TreeNodeWithValue<int>* f4 = new TreeNodeWithValue<int>(4);
+  f3->Add(f4, 0);
+  f4->Add(new TreeNodeWithValue<int>(5), 0);
+  f3->Add(new TreeNodeWithValue<int>(7), 1);
+
+  TreeNodeIterator<TreeNodeWithValue<int> > oddIterator(&root, PruneOdd);
+  ASSERT_TRUE(oddIterator.has_next());
+  ASSERT_EQ(2, oddIterator.Next()->value);
+  ASSERT_FALSE(oddIterator.has_next());
+
+  TreeNodeIterator<TreeNodeWithValue<int> > evenIterator(&root, PruneEven);
+  ASSERT_TRUE(evenIterator.has_next());
+  ASSERT_EQ(1, evenIterator.Next()->value);
+  ASSERT_TRUE(evenIterator.has_next());
+  ASSERT_EQ(3, evenIterator.Next()->value);
+  ASSERT_TRUE(evenIterator.has_next());
+  ASSERT_EQ(7, evenIterator.Next()->value);
+  ASSERT_FALSE(evenIterator.has_next());
+}
+
 }  // namespace ui