Makes adding a new folder from the bookmark context menu add the

folder right after the selected item (when possible) rather than as
the last element.

BUG=9240
TEST=see bug

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

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

6 files changed, 85 insertions, 53 deletions

diff --git a/chrome/browser/bookmarks/bookmark_context_menu_controller.cc b/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
index 8b7b46e..2060a51 100644
--- a/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
+++ b/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
@@ -52,11 +52,13 @@ class EditFolderController : public InputWindowDialog::Delegate,
   static void Show(Profile* profile,
                    gfx::NativeWindow wnd,
                    const BookmarkNode* node,
+                   int index,
                    bool is_new,
                    bool show_in_manager) {
     // EditFolderController deletes itself when done.
     EditFolderController* controller =
-        new EditFolderController(profile, wnd, node, is_new, show_in_manager);
+        new EditFolderController(profile, wnd, node, index, is_new,
+                                 show_in_manager);
     controller->Show();
   }
 
@@ -64,11 +66,13 @@ class EditFolderController : public InputWindowDialog::Delegate,
   EditFolderController(Profile* profile,
                        gfx::NativeWindow wnd,
                        const BookmarkNode* node,
+                       int index,
                        bool is_new,
                        bool show_in_manager)
       : profile_(profile),
         model_(profile->GetBookmarkModel()),
         node_(node),
+        index_(index),
         is_new_(is_new),
         show_in_manager_(show_in_manager) {
     DCHECK(is_new_ || node);
@@ -98,10 +102,9 @@ class EditFolderController : public InputWindowDialog::Delegate,
   virtual void InputAccepted(const std::wstring& text) {
     if (is_new_) {
       ALLOW_UNUSED const BookmarkNode* node =
-          model_->AddGroup(node_, node_->GetChildCount(), text);
-      if (show_in_manager_) {
+          model_->AddGroup(node_, index_, text);
+      if (show_in_manager_)
         BookmarkManager::SelectInTree(profile_, node);
-      }
     } else {
       model_->SetTitle(node_, text);
     }
@@ -163,6 +166,9 @@ class EditFolderController : public InputWindowDialog::Delegate,
   // Otherwise this is the node to change the title of.
   const BookmarkNode* node_;
 
+  // Index to insert the new folder at.
+  int index_;
+
   bool is_new_;
 
   // If is_new_ is true and a new node is created, it is selected in the
@@ -344,7 +350,7 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
                              editor_config, NULL);
       } else {
         EditFolderController::Show(profile_, parent_window_, selection_[0],
-                                   false, false);
+                                   -1, false, false);
       }
       break;
 
@@ -375,9 +381,12 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
         // This is owned by the BookmarkEditorView.
         handler = new SelectOnCreationHandler(profile_);
       }
-      BookmarkEditor::Show(parent_window_, profile_, GetParentForNewNodes(),
-                           BookmarkEditor::EditDetails(), editor_config,
-                           handler);
+      // TODO: this should honor the index from GetParentForNewNodes.
+      BookmarkEditor::Show(
+          parent_window_, profile_,
+          bookmark_utils::GetParentForNewNodes(parent_, selection_, NULL),
+          BookmarkEditor::EditDetails(), editor_config,
+          handler);
       break;
     }
 
@@ -385,8 +394,10 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
       UserMetrics::RecordAction(
           UserMetricsAction("BookmarkBar_ContextMenu_NewFolder"),
           profile_);
-      EditFolderController::Show(profile_, parent_window_,
-                                 GetParentForNewNodes(), true,
+      int index;
+      const BookmarkNode* parent =
+          bookmark_utils::GetParentForNewNodes(parent_, selection_, &index);
+      EditFolderController::Show(profile_, parent_window_, parent, index, true,
                                  configuration_ != BOOKMARK_BAR);
       break;
     }
@@ -435,14 +446,12 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
       break;
 
     case IDS_PASTE: {
-      const BookmarkNode* paste_target = GetParentForNewNodes();
+      int index;
+      const BookmarkNode* paste_target =
+          bookmark_utils::GetParentForNewNodes(parent_, selection_, &index);
       if (!paste_target)
         return;
 
-      int index = -1;
-      if (selection_.size() == 1 && selection_[0]->is_url())
-        index = paste_target->IndexOfChild(selection_[0]) + 1;
-
       bookmark_utils::PasteFromClipboard(model_, parent_, index);
       break;
     }
@@ -492,7 +501,8 @@ bool BookmarkContextMenuController::IsCommandIdEnabled(int command_id) const {
 
     case IDS_BOOMARK_BAR_NEW_FOLDER:
     case IDS_BOOMARK_BAR_ADD_NEW_BOOKMARK:
-      return GetParentForNewNodes() != NULL;
+      return bookmark_utils::GetParentForNewNodes(
+          parent_, selection_, NULL) != NULL;
 
     case IDS_COPY:
     case IDS_CUT:
@@ -560,9 +570,3 @@ bool BookmarkContextMenuController::HasURLs() const {
   }
   return false;
 }
-
-const BookmarkNode*
-    BookmarkContextMenuController::GetParentForNewNodes() const {
-  return (selection_.size() == 1 && selection_[0]->is_folder()) ?
-      selection_[0] : parent_;
-}
diff --git a/chrome/browser/bookmarks/bookmark_context_menu_controller.h b/chrome/browser/bookmarks/bookmark_context_menu_controller.h
index b2776df..ab3f936 100644
--- a/chrome/browser/bookmarks/bookmark_context_menu_controller.h
+++ b/chrome/browser/bookmarks/bookmark_context_menu_controller.h
@@ -128,11 +128,6 @@ class BookmarkContextMenuController : public BookmarkModelObserver,
   // Returns true if selection_ has at least one bookmark of type url.
   bool HasURLs() const;
 
-  // Returns the parent for newly created folders/bookmarks. If selection_
-  // has one element and it is a folder, selection_[0] is returned, otherwise
-  // parent_ is returned.
-  const BookmarkNode* GetParentForNewNodes() const;
-
   gfx::NativeWindow parent_window_;
   BookmarkContextMenuControllerDelegate* delegate_;
   Profile* profile_;
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc
index cda4ef0..a9aa9b6 100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -655,4 +655,29 @@ void GetURLsForOpenTabs(Browser* browser,
   }
 }
 
+const BookmarkNode* GetParentForNewNodes(
+    const BookmarkNode* parent,
+    const std::vector<const BookmarkNode*>& selection,
+    int* index) {
+  const BookmarkNode* real_parent = parent;
+
+  if (selection.size() == 1 && selection[0]->is_folder())
+    real_parent = selection[0];
+
+  if (index) {
+    if (selection.size() == 1 && selection[0]->is_url()) {
+      *index = real_parent->IndexOfChild(selection[0]) + 1;
+      if (*index == 0) {
+        // Node doesn't exist in parent, add to end.
+        NOTREACHED();
+        *index = real_parent->GetChildCount();
+      }
+    } else {
+      *index = real_parent->GetChildCount();
+    }
+  }
+
+  return real_parent;
+}
+
 }  // namespace bookmark_utils
diff --git a/chrome/browser/bookmarks/bookmark_utils.h b/chrome/browser/bookmarks/bookmark_utils.h
index db1bfda..ed7f850 100644
--- a/chrome/browser/bookmarks/bookmark_utils.h
+++ b/chrome/browser/bookmarks/bookmark_utils.h
@@ -200,6 +200,15 @@ void GetURLAndTitleToBookmark(TabContents* tab_contents,
 void GetURLsForOpenTabs(Browser* browser,
                         std::vector<std::pair<GURL, std::wstring> >* urls);
 
+// Returns the parent for newly created folders/bookmarks. If |selection| has
+// one element and it is a folder, |selection[0]| is returned, otherwise
+// |parent| is returned. If |index| is non-null it is set to the index newly
+// added nodes should be added at.
+const BookmarkNode* GetParentForNewNodes(
+    const BookmarkNode* parent,
+    const std::vector<const BookmarkNode*>& selection,
+    int* index);
+
 // Number of bookmarks we'll open before prompting the user to see if they
 // really want to open all.
 //
diff --git a/chrome/browser/views/bookmark_context_menu_controller_views.cc b/chrome/browser/views/bookmark_context_menu_controller_views.cc
index 1cae184..a208233 100644
--- a/chrome/browser/views/bookmark_context_menu_controller_views.cc
+++ b/chrome/browser/views/bookmark_context_menu_controller_views.cc
@@ -52,11 +52,13 @@ class EditFolderController : public InputWindowDialog::Delegate,
   static void Show(Profile* profile,
                    gfx::NativeWindow wnd,
                    const BookmarkNode* node,
+                   int index,
                    bool is_new,
                    bool show_in_manager) {
     // EditFolderController deletes itself when done.
     EditFolderController* controller =
-        new EditFolderController(profile, wnd, node, is_new, show_in_manager);
+        new EditFolderController(profile, wnd, node, index, is_new,
+                                 show_in_manager);
     controller->Show();
   }
 
@@ -64,11 +66,13 @@ class EditFolderController : public InputWindowDialog::Delegate,
   EditFolderController(Profile* profile,
                        gfx::NativeWindow wnd,
                        const BookmarkNode* node,
+                       int index,
                        bool is_new,
                        bool show_in_manager)
       : profile_(profile),
         model_(profile->GetBookmarkModel()),
         node_(node),
+        index_(index),
         is_new_(is_new),
         show_in_manager_(show_in_manager) {
     DCHECK(is_new_ || node);
@@ -98,10 +102,9 @@ class EditFolderController : public InputWindowDialog::Delegate,
   virtual void InputAccepted(const std::wstring& text) {
     if (is_new_) {
       ALLOW_UNUSED const BookmarkNode* node =
-          model_->AddGroup(node_, node_->GetChildCount(), text);
-      if (show_in_manager_) {
+          model_->AddGroup(node_, index_, text);
+      if (show_in_manager_)
         BookmarkManager::SelectInTree(profile_, node);
-      }
     } else {
       model_->SetTitle(node_, text);
     }
@@ -163,6 +166,9 @@ class EditFolderController : public InputWindowDialog::Delegate,
   // Otherwise this is the node to change the title of.
   const BookmarkNode* node_;
 
+  // Index to insert the new folder at.
+  int index_;
+
   bool is_new_;
 
   // If is_new_ is true and a new node is created, it is selected in the
@@ -325,7 +331,7 @@ void BookmarkContextMenuControllerViews::ExecuteCommand(int id) {
                              editor_config, NULL);
       } else {
         EditFolderController::Show(profile_, parent_window_, selection_[0],
-                                   false, false);
+                                   -1, false, false);
       }
       break;
 
@@ -356,9 +362,11 @@ void BookmarkContextMenuControllerViews::ExecuteCommand(int id) {
         // This is owned by the BookmarkEditorView.
         handler = new SelectOnCreationHandler(profile_);
       }
-      BookmarkEditor::Show(parent_window_, profile_, GetParentForNewNodes(),
-                           BookmarkEditor::EditDetails(), editor_config,
-                           handler);
+      // TODO: this should honor the index from GetParentForNewNodes.
+      BookmarkEditor::Show(
+          parent_window_, profile_,
+          bookmark_utils::GetParentForNewNodes(parent_, selection_, NULL),
+          BookmarkEditor::EditDetails(), editor_config, handler);
       break;
     }
 
@@ -366,8 +374,11 @@ void BookmarkContextMenuControllerViews::ExecuteCommand(int id) {
       UserMetrics::RecordAction(
           UserMetricsAction("BookmarkBar_ContextMenu_NewFolder"),
           profile_);
+      int index;
+      const BookmarkNode* parent =
+          bookmark_utils::GetParentForNewNodes(parent_, selection_, &index);
       EditFolderController::Show(profile_, parent_window_,
-                                 GetParentForNewNodes(), true,
+                                 parent, index, true,
                                  configuration_ != BOOKMARK_BAR);
       break;
     }
@@ -418,14 +429,12 @@ void BookmarkContextMenuControllerViews::ExecuteCommand(int id) {
       break;
 
     case IDS_PASTE: {
-      const BookmarkNode* paste_target = GetParentForNewNodes();
+      int index;
+      const BookmarkNode* paste_target =
+          bookmark_utils::GetParentForNewNodes(parent_, selection_, &index);
       if (!paste_target)
         return;
 
-      int index = -1;
-      if (selection_.size() == 1 && selection_[0]->is_url())
-        index = paste_target->IndexOfChild(selection_[0]) + 1;
-
       bookmark_utils::PasteFromClipboard(model, parent_, index);
       break;
     }
@@ -472,7 +481,8 @@ bool BookmarkContextMenuControllerViews::IsCommandEnabled(int id) const {
 
     case IDS_BOOMARK_BAR_NEW_FOLDER:
     case IDS_BOOMARK_BAR_ADD_NEW_BOOKMARK:
-      return GetParentForNewNodes() != NULL;
+      return bookmark_utils::GetParentForNewNodes(
+          parent_, selection_, NULL) != NULL;
 
     case IDS_COPY:
     case IDS_CUT:
@@ -546,9 +556,3 @@ bool BookmarkContextMenuControllerViews::HasURLs() const {
   }
   return false;
 }
-
-const BookmarkNode*
-    BookmarkContextMenuControllerViews::GetParentForNewNodes() const {
-  return (selection_.size() == 1 && selection_[0]->is_folder()) ?
-      selection_[0] : parent_;
-}
diff --git a/chrome/browser/views/bookmark_context_menu_controller_views.h b/chrome/browser/views/bookmark_context_menu_controller_views.h
index 46a8ecc..20453b8 100644
--- a/chrome/browser/views/bookmark_context_menu_controller_views.h
+++ b/chrome/browser/views/bookmark_context_menu_controller_views.h
@@ -116,11 +116,6 @@ class BookmarkContextMenuControllerViews : public BookmarkModelObserver {
   // Returns true if selection_ has at least one bookmark of type url.
   bool HasURLs() const;
 
-  // Returns the parent for newly created folders/bookmarks. If selection_
-  // has one element and it is a folder, selection_[0] is returned, otherwise
-  // parent_ is returned.
-  const BookmarkNode* GetParentForNewNodes() const;
-
   gfx::NativeWindow parent_window_;
   BookmarkContextMenuControllerViewsDelegate* delegate_;
   Profile* profile_;