Wires up sorting of bookmarks to the 'organize menu' in the bookmark

manager (Glen says no context menus for now). All
BookmarkModelObservers have been updated appropriately.

BUG=1750
TEST=bring up the bookmark manager and try the 'Reorder by title' menu
  item, make sure it works and I didn't screw up anything around it.

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

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

10 files changed, 148 insertions, 14 deletions

diff --git a/chrome/browser/bookmarks/bookmark_context_menu.cc b/chrome/browser/bookmarks/bookmark_context_menu.cc
index 727d16a..2c58710 100644
--- a/chrome/browser/bookmarks/bookmark_context_menu.cc
+++ b/chrome/browser/bookmarks/bookmark_context_menu.cc
@@ -165,6 +165,11 @@ class EditFolderController : public InputWindowDelegate,
   virtual void BookmarkNodeFavIconLoaded(BookmarkModel* model,
                                          BookmarkNode* node) {}
 
+  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                             BookmarkNode* node) {
+    ModelChanged();
+  }
+
   void ModelChanged() {
     window_->Close();
   }
@@ -290,6 +295,13 @@ BookmarkContextMenu::BookmarkContextMenu(
         IDS_PASTE, l10n_util::GetString(IDS_PASTE));
   }
 
+  if (configuration == BOOKMARK_MANAGER_ORGANIZE_MENU) {
+    menu_->AppendSeparator();
+    menu_->AppendMenuItemWithLabel(
+        IDS_BOOKMARK_MANAGER_SORT,
+        l10n_util::GetString(IDS_BOOKMARK_MANAGER_SORT));
+  }
+
   menu_->AppendSeparator();
 
   menu_->AppendMenuItemWithLabel(
@@ -437,6 +449,11 @@ void BookmarkContextMenu::ExecuteCommand(int id) {
       BookmarkManagerView::Show(profile_);
       break;
 
+    case IDS_BOOKMARK_MANAGER_SORT:
+      UserMetrics::RecordAction(L"BookmarkManager_Sort", profile_);
+      model_->SortChildren(parent_);
+      break;
+
     case IDS_COPY:
     case IDS_CUT:
       bookmark_utils::CopyToClipboard(profile_->GetBookmarkModel(),
@@ -494,6 +511,9 @@ bool BookmarkContextMenu::IsCommandEnabled(int id) const {
               configuration_ == BOOKMARK_MANAGER_ORGANIZE_MENU_OTHER) &&
              selection_.size() == 1;
 
+    case IDS_BOOKMARK_MANAGER_SORT:
+      return parent_ && parent_ != model_->root_node();
+
     case IDS_BOOMARK_BAR_NEW_FOLDER:
     case IDS_BOOMARK_BAR_ADD_NEW_BOOKMARK:
       return GetParentForNewNodes() != NULL;
@@ -539,6 +559,11 @@ void BookmarkContextMenu::BookmarkNodeChanged(BookmarkModel* model,
   ModelChanged();
 }
 
+void BookmarkContextMenu::BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                                        BookmarkNode* node) {
+  ModelChanged();
+}
+
 void BookmarkContextMenu::ModelChanged() {
   menu_->Cancel();
 }
diff --git a/chrome/browser/bookmarks/bookmark_context_menu.h b/chrome/browser/bookmarks/bookmark_context_menu.h
index f0408f3..1d08110 100644
--- a/chrome/browser/bookmarks/bookmark_context_menu.h
+++ b/chrome/browser/bookmarks/bookmark_context_menu.h
@@ -89,6 +89,8 @@ class BookmarkContextMenu : public views::MenuDelegate,
                                    BookmarkNode* node);
   virtual void BookmarkNodeFavIconLoaded(BookmarkModel* model,
                                          BookmarkNode* node) {}
+  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                             BookmarkNode* node);
 
   // Invoked from the various bookmark model observer methods. Closes the menu.
   void ModelChanged();
diff --git a/chrome/browser/bookmarks/bookmark_folder_tree_model.cc b/chrome/browser/bookmarks/bookmark_folder_tree_model.cc
index a743255..de09b13 100644
--- a/chrome/browser/bookmarks/bookmark_folder_tree_model.cc
+++ b/chrome/browser/bookmarks/bookmark_folder_tree_model.cc
@@ -130,6 +130,40 @@ void BookmarkFolderTreeModel::BookmarkNodeChanged(BookmarkModel* model,
     GetObserver()->TreeNodeChanged(this, folder_node);
 }
 
+void BookmarkFolderTreeModel::BookmarkNodeChildrenReordered(
+    BookmarkModel* model,
+    BookmarkNode* node) {
+  FolderNode* folder_node = GetFolderNodeForBookmarkNode(node);
+  DCHECK(folder_node);
+  if (folder_node->GetChildCount() <= 1)
+    return;  // Order won't have changed if 1 or fewer nodes.
+
+  // Build a map between folder node and bookmark node.
+  std::map<BookmarkNode*, FolderNode*> bn_to_folder;
+  for (int i = 0; i < folder_node->GetChildCount(); ++i)
+    bn_to_folder[folder_node->GetChild(i)->value] = folder_node->GetChild(i);
+
+  // Remove all the folder nodes.
+  int original_count = folder_node->GetChildCount();
+  folder_node->RemoveAll();
+
+  // And add them back in the new order.
+  for (int i = 0; i < node->GetChildCount(); ++i) {
+    BookmarkNode* child = node->GetChild(i);
+    if (child->is_folder()) {
+      DCHECK(bn_to_folder.find(child) != bn_to_folder.end());
+      folder_node->Add(folder_node->GetChildCount(), bn_to_folder[child]);
+    }
+  }
+  // The new count better match the original count, otherwise we're leaking and
+  // treeview is likely to get way out of sync.
+  DCHECK(original_count == folder_node->GetChildCount());
+
+  // Finally, notify observers.
+  if (GetObserver())
+    GetObserver()->TreeNodeChildrenReordered(this, folder_node);
+}
+
 void BookmarkFolderTreeModel::GetIcons(std::vector<SkBitmap>* icons) {
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   icons->push_back(*rb.GetBitmapNamed(IDR_BOOKMARK_MANAGER_RECENT_ICON));
diff --git a/chrome/browser/bookmarks/bookmark_folder_tree_model.h b/chrome/browser/bookmarks/bookmark_folder_tree_model.h
index acf49f4..e82c114 100644
--- a/chrome/browser/bookmarks/bookmark_folder_tree_model.h
+++ b/chrome/browser/bookmarks/bookmark_folder_tree_model.h
@@ -64,6 +64,8 @@ class BookmarkFolderTreeModel : public views::TreeNodeModel<FolderNode>,
                                    BookmarkNode* node);
   virtual void BookmarkNodeChanged(BookmarkModel* model,
                                    BookmarkNode* node);
+  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                             BookmarkNode* node);
   // Folders don't have favicons, so we ignore this.
   virtual void BookmarkNodeFavIconLoaded(BookmarkModel* model,
                                          BookmarkNode* node) {}
diff --git a/chrome/browser/bookmarks/bookmark_folder_tree_model_unittest.cc b/chrome/browser/bookmarks/bookmark_folder_tree_model_unittest.cc
index ac0037b..97f4feb 100644
--- a/chrome/browser/bookmarks/bookmark_folder_tree_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_folder_tree_model_unittest.cc
@@ -20,6 +20,8 @@
 //   url2
 //   f2
 //   url3
+//   a1
+//   g1
 class BookmarkFolderTreeModelTest : public testing::Test,
                                     public views::TreeModelObserver {
  public:
@@ -29,7 +31,8 @@ class BookmarkFolderTreeModelTest : public testing::Test,
         url3_("http://3"),
         added_count_(0),
         removed_count_(0),
-        changed_count_(0) {
+        changed_count_(0),
+        reordered_count_(0) {
    }
 
   virtual void SetUp() {
@@ -78,16 +81,24 @@ class BookmarkFolderTreeModelTest : public testing::Test,
     changed_count_++;
   }
 
-  void VerifyAndClearObserverCounts(int changed_count, int added_count,
-                                      int removed_count) {
+  virtual void TreeNodeChildrenReordered(views::TreeModel* model,
+                                         views::TreeModelNode* parent) {
+    reordered_count_++;
+  }
+
+  void VerifyAndClearObserverCounts(int changed_count,
+                                    int added_count,
+                                    int removed_count,
+                                    int reordered_count) {
     EXPECT_EQ(changed_count, changed_count_);
     EXPECT_EQ(added_count, added_count_);
     EXPECT_EQ(removed_count, removed_count_);
+    EXPECT_EQ(reordered_count, reordered_count_);
     ResetCounts();
   }
 
   void ResetCounts() {
-    changed_count_ = removed_count_ = added_count_ = 0;
+    changed_count_ = removed_count_ = added_count_ = reordered_count_ = 0;
   }
 
   scoped_ptr<BookmarkFolderTreeModel> model_;
@@ -100,6 +111,7 @@ class BookmarkFolderTreeModelTest : public testing::Test,
   int changed_count_;
   int added_count_;
   int removed_count_;
+  int reordered_count_;
   scoped_ptr<TestingProfile> profile_;
 };
 
@@ -138,27 +150,27 @@ TEST_F(BookmarkFolderTreeModelTest, InitialState) {
 // Removes a URL node and makes sure we don't get any notification.
 TEST_F(BookmarkFolderTreeModelTest, RemoveURL) {
   bookmark_model()->Remove(bookmark_model()->GetBookmarkBarNode(), 0);
-  VerifyAndClearObserverCounts(0, 0, 0);
+  VerifyAndClearObserverCounts(0, 0, 0, 0);
 }
 
 // Changes the title of a URL and makes sure we don't get any notification.
 TEST_F(BookmarkFolderTreeModelTest, ChangeURL) {
   bookmark_model()->SetTitle(
       bookmark_model()->GetBookmarkBarNode()->GetChild(0), L"BLAH");
-  VerifyAndClearObserverCounts(0, 0, 0);
+  VerifyAndClearObserverCounts(0, 0, 0, 0);
 }
 
 // Adds a URL and make sure we don't get notification.
 TEST_F(BookmarkFolderTreeModelTest, AddURL) {
   bookmark_model()->AddURL(
       bookmark_model()->other_node(), 0, L"url1", url1_);
-  VerifyAndClearObserverCounts(0, 0, 0);
+  VerifyAndClearObserverCounts(0, 0, 0, 0);
 }
 
 // Removes a folder and makes sure we get the right notification.
 TEST_F(BookmarkFolderTreeModelTest, RemoveFolder) {
   bookmark_model()->Remove(bookmark_model()->GetBookmarkBarNode(), 1);
-  VerifyAndClearObserverCounts(0, 0, 1);
+  VerifyAndClearObserverCounts(0, 0, 1, 0);
   // Make sure the node was removed.
   EXPECT_EQ(0, model_->GetRoot()->GetChild(0)->GetChildCount());
 }
@@ -168,7 +180,7 @@ TEST_F(BookmarkFolderTreeModelTest, AddFolder) {
   BookmarkNode* new_group =
       bookmark_model()->AddGroup(
           bookmark_model()->GetBookmarkBarNode(), 0, L"fa");
-  VerifyAndClearObserverCounts(0, 1, 0);
+  VerifyAndClearObserverCounts(0, 1, 0, 0);
   // Make sure the node was added at the right place.
   // Make sure the node was removed.
   ASSERT_EQ(2, model_->GetRoot()->GetChild(0)->GetChildCount());
@@ -181,5 +193,27 @@ TEST_F(BookmarkFolderTreeModelTest, ChangeFolder) {
   bookmark_model()->SetTitle(
       bookmark_model()->GetBookmarkBarNode()->GetChild(1)->GetChild(0),
       L"BLAH");
-  VerifyAndClearObserverCounts(1, 0, 0);
+  VerifyAndClearObserverCounts(1, 0, 0, 0);
+}
+
+// Sorts the other folder, making sure the resulting order is correct and the
+// appropriate notification is sent.
+TEST_F(BookmarkFolderTreeModelTest, Sort) {
+  BookmarkNode* other = bookmark_model()->other_node();
+  bookmark_model()->AddGroup(other, 3, L"a1");
+  bookmark_model()->AddGroup(other, 4, L"g1");
+  ResetCounts();
+
+  bookmark_model()->SortChildren(other);
+
+  // Make sure we got notification.
+  VerifyAndClearObserverCounts(0, 0, 0, 1);
+
+  // Make sure the resulting order matches.
+  FolderNode* other_folder_node =
+      model_->GetFolderNodeForBookmarkNode(bookmark_model()->other_node());
+  ASSERT_EQ(3, other_folder_node->GetChildCount());
+  EXPECT_TRUE(other_folder_node->GetChild(0)->GetTitle() == L"a1");
+  EXPECT_TRUE(other_folder_node->GetChild(1)->GetTitle() == L"f2");
+  EXPECT_TRUE(other_folder_node->GetChild(2)->GetTitle() == L"g1");
 }
diff --git a/chrome/browser/bookmarks/bookmark_model.h b/chrome/browser/bookmarks/bookmark_model.h
index a886725..23517dd 100644
--- a/chrome/browser/bookmarks/bookmark_model.h
+++ b/chrome/browser/bookmarks/bookmark_model.h
@@ -172,9 +172,8 @@ class BookmarkModelObserver {
 
   // Invoked when the children (just direct children, not descendants) of
   // |node| have been reordered in some way, such as sorted.
-  // TODO(sky): make this pure virtual when all observers have been updated.
   virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
-                                             BookmarkNode* node) {}
+                                             BookmarkNode* node) = 0;
 };
 
 // BookmarkModel --------------------------------------------------------------
diff --git a/chrome/browser/bookmarks/bookmark_model_unittest.cc b/chrome/browser/bookmarks/bookmark_model_unittest.cc
index e2fb371..1fabdbd 100644
--- a/chrome/browser/bookmarks/bookmark_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_model_unittest.cc
@@ -656,6 +656,8 @@ class BookmarkModelTestWithProfile : public testing::Test,
                                    int index) {}
   virtual void BookmarkNodeChanged(BookmarkModel* model,
                                    BookmarkNode* node) {}
+  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                             BookmarkNode* node) {}
   virtual void BookmarkNodeFavIconLoaded(BookmarkModel* model,
                                          BookmarkNode* node) {}
 
diff --git a/chrome/browser/bookmarks/bookmark_table_model.cc b/chrome/browser/bookmarks/bookmark_table_model.cc
index 99c2f29..7a50e29 100644
--- a/chrome/browser/bookmarks/bookmark_table_model.cc
+++ b/chrome/browser/bookmarks/bookmark_table_model.cc
@@ -85,8 +85,7 @@ class FolderBookmarkTableModel : public VectorBackedBookmarkTableModel {
   FolderBookmarkTableModel(BookmarkModel* model, BookmarkNode* root_node)
       : VectorBackedBookmarkTableModel(model),
         root_node_(root_node) {
-    for (int i = 0; i < root_node->GetChildCount(); ++i)
-      nodes().push_back(root_node->GetChild(i));
+    PopulateNodesFromRoot();
   }
 
   virtual void BookmarkNodeMoved(BookmarkModel* model,
@@ -146,12 +145,29 @@ class FolderBookmarkTableModel : public VectorBackedBookmarkTableModel {
     NotifyChanged(node);
   }
 
+  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                             BookmarkNode* node) {
+    if (node != root_node_)
+      return;
+
+    nodes().clear();
+    PopulateNodesFromRoot();
+
+    if (observer())
+      observer()->OnModelChanged();
+  }
+
  private:
   void NotifyChanged(BookmarkNode* node) {
     if (node->GetParent() == root_node_ && observer())
       observer()->OnItemsChanged(node->GetParent()->IndexOfChild(node), 1);
   }
 
+  void PopulateNodesFromRoot() {
+    for (int i = 0; i < root_node_->GetChildCount(); ++i)
+      nodes().push_back(root_node_->GetChild(i));
+  }
+
   // The node we're showing the children of. This is set to NULL if the node
   // (or one of its ancestors) is removed from the model.
   BookmarkNode* root_node_;
diff --git a/chrome/browser/bookmarks/bookmark_table_model.h b/chrome/browser/bookmarks/bookmark_table_model.h
index 55c557d..d5b4a2b 100644
--- a/chrome/browser/bookmarks/bookmark_table_model.h
+++ b/chrome/browser/bookmarks/bookmark_table_model.h
@@ -46,6 +46,9 @@ class BookmarkTableModel : public views::TableModel,
   // BookmarkModelObserver methods.
   virtual void Loaded(BookmarkModel* model) {}
   virtual void BookmarkModelBeingDeleted(BookmarkModel* model);
+  virtual void BookmarkNodeChildrenReordered(BookmarkModel* model,
+                                             BookmarkNode* node) {}
+
 
   // Returns the index of the specified node, or -1 if the node isn't in the
   // model.
diff --git a/chrome/browser/bookmarks/bookmark_table_model_unittest.cc b/chrome/browser/bookmarks/bookmark_table_model_unittest.cc
index b709b28..7e4182c 100644
--- a/chrome/browser/bookmarks/bookmark_table_model_unittest.cc
+++ b/chrome/browser/bookmarks/bookmark_table_model_unittest.cc
@@ -143,6 +143,23 @@ TEST_F(BookmarkTableModelTest, AddToFolder) {
   VerifyAndClearOberserverCounts(0, 0, 0, 0);
 }
 
+// Verifies sort sends out notification and results in a sort.
+TEST_F(BookmarkTableModelTest, SortFolder) {
+  BookmarkNode* other = bookmark_model()->other_node();
+  SetModel(BookmarkTableModel::CreateBookmarkTableModelForFolder(
+           bookmark_model(), other));
+  ASSERT_EQ(3, model_->RowCount());
+  bookmark_model()->SortChildren(other);
+
+  // Sorting should trigger change notification.
+  VerifyAndClearOberserverCounts(1, 0, 0, 0);
+
+  // Make sure things reordered.
+  EXPECT_TRUE(other->GetChild(0) == model_->GetNodeForRow(0));
+  EXPECT_TRUE(other->GetChild(1) == model_->GetNodeForRow(1));
+  EXPECT_TRUE(other->GetChild(2) == model_->GetNodeForRow(2));
+}
+
 // Verifies removing an item from folder model generates the correct event.
 TEST_F(BookmarkTableModelTest, RemoveFromFolder) {
   BookmarkNode* other = bookmark_model()->other_node();