Makes canceling 'bookmark all tabs' delete the folder. Or rather,

makes it so that bookmark all tabs only creates the folder if the user
presses ok. I wasn't happy adding another random arg to
BookmarkEditor::Show, so I added in a structure an enum. This makes it
clearer what Show should do.

I also fixed the following:
. On gtk we wouldn't always pick the right parent for nodes.
. The context menu item is now enabled on views/gtk.

And this now breaks the mac side. I'll straighten that out right after
landing this.

BUG=24367
TEST=Make sure 'bookmark all tabs' works, as well as the bookmark
editor work. (get to the bookmark editor by creating a new bookmark,
then clicking edit, or right clicking a bookmark on the bookmark bar and
choosing edit).

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

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

8 files changed, 77 insertions, 63 deletions

diff --git a/chrome/browser/gtk/bookmark_bubble_gtk.cc b/chrome/browser/gtk/bookmark_bubble_gtk.cc
index c43a7a0..a17db56 100644
--- a/chrome/browser/gtk/bookmark_bubble_gtk.cc
+++ b/chrome/browser/gtk/bookmark_bubble_gtk.cc
@@ -369,7 +369,8 @@ void BookmarkBubbleGtk::ShowEditor() {
   bubble_->Close();
 
   if (node) {
-    BookmarkEditor::Show(toplevel, profile, NULL, node,
+    BookmarkEditor::Show(toplevel, profile, NULL,
+                         BookmarkEditor::EditDetails(node),
                          BookmarkEditor::SHOW_TREE, NULL);
   }
 }
diff --git a/chrome/browser/gtk/bookmark_context_menu_gtk.cc b/chrome/browser/gtk/bookmark_context_menu_gtk.cc
index 9c3f03c..b0b2819 100644
--- a/chrome/browser/gtk/bookmark_context_menu_gtk.cc
+++ b/chrome/browser/gtk/bookmark_context_menu_gtk.cc
@@ -338,7 +338,8 @@ void BookmarkContextMenuGtk::ExecuteCommand(int id) {
           editor_config = BookmarkEditor::SHOW_TREE;
         else
           editor_config = BookmarkEditor::NO_TREE;
-        BookmarkEditor::Show(wnd_, profile_, parent_, selection_[0],
+        BookmarkEditor::Show(wnd_, profile_, parent_,
+                             BookmarkEditor::EditDetails(selection_[0]),
                              editor_config, NULL);
       } else {
         EditFolderController::Show(profile_, wnd_, selection_[0], false,
@@ -370,8 +371,9 @@ void BookmarkContextMenuGtk::ExecuteCommand(int id) {
         // This is owned by the BookmarkEditorView.
         handler = new SelectOnCreationHandler(profile_);
       }
-      BookmarkEditor::Show(wnd_, profile_, GetParentForNewNodes(), NULL,
-                           editor_config, handler);
+      BookmarkEditor::Show(wnd_, profile_, GetParentForNewNodes(),
+                           BookmarkEditor::EditDetails(), editor_config,
+                           handler);
       break;
     }
 
diff --git a/chrome/browser/gtk/bookmark_editor_gtk.cc b/chrome/browser/gtk/bookmark_editor_gtk.cc
index 9b95b66..6b32462 100644
--- a/chrome/browser/gtk/bookmark_editor_gtk.cc
+++ b/chrome/browser/gtk/bookmark_editor_gtk.cc
@@ -40,13 +40,13 @@ static const int kTreeHeight = 150;
 void BookmarkEditor::Show(gfx::NativeWindow parent_hwnd,
                           Profile* profile,
                           const BookmarkNode* parent,
-                          const BookmarkNode* node,
+                          const EditDetails& details,
                           Configuration configuration,
                           Handler* handler) {
   DCHECK(profile);
   BookmarkEditorGtk* editor =
-      new BookmarkEditorGtk(parent_hwnd, profile, parent, node, configuration,
-                            handler);
+      new BookmarkEditorGtk(parent_hwnd, profile, parent, details,
+                            configuration, handler);
   editor->Show();
 }
 
@@ -54,13 +54,13 @@ BookmarkEditorGtk::BookmarkEditorGtk(
     GtkWindow* window,
     Profile* profile,
     const BookmarkNode* parent,
-    const BookmarkNode* node,
+    const EditDetails& details,
     BookmarkEditor::Configuration configuration,
     BookmarkEditor::Handler* handler)
     : profile_(profile),
       dialog_(NULL),
       parent_(parent),
-      node_(node),
+      details_(details),
       running_menu_for_root_(false),
       show_tree_(configuration == SHOW_TREE),
       handler_(handler) {
@@ -125,24 +125,32 @@ void BookmarkEditorGtk::Init(GtkWindow* parent_window) {
   // |+-------------------------------------------------------------+|
   // +---------------------------------------------------------------+
   //
-  // * The url and corresponding label are not shown if node_ is a folder.
+  // * The url and corresponding label are not shown if creating a new folder.
   GtkWidget* content_area = GTK_DIALOG(dialog_)->vbox;
   gtk_box_set_spacing(GTK_BOX(content_area), gtk_util::kContentAreaSpacing);
 
   GtkWidget* vbox = gtk_vbox_new(FALSE, 12);
 
   name_entry_ = gtk_entry_new();
-  gtk_entry_set_text(GTK_ENTRY(name_entry_),
-                     node_ ? WideToUTF8(node_->GetTitle()).c_str() : "");
+  std::string title;
+  if (details_.type == EditDetails::EXISTING_NODE) {
+    title = WideToUTF8(details_.existing_node->GetTitle());
+  } else if (details_.type == EditDetails::NEW_FOLDER) {
+    title = WideToUTF8(
+        l10n_util::GetString(IDS_BOOMARK_EDITOR_NEW_FOLDER_NAME));
+  }
+  gtk_entry_set_text(GTK_ENTRY(name_entry_), title.c_str());
   g_signal_connect(G_OBJECT(name_entry_), "changed",
                    G_CALLBACK(OnEntryChanged), this);
   gtk_entry_set_activates_default(GTK_ENTRY(name_entry_), TRUE);
 
   GtkWidget* table;
-  if (!IsEditingFolder()) {
+  if (details_.type != EditDetails::NEW_FOLDER) {
     url_entry_ = gtk_entry_new();
-    gtk_entry_set_text(GTK_ENTRY(url_entry_),
-                       node_ ? node_->GetURL().spec().c_str() : "");
+    std::string url_spec;
+    if (details_.type == EditDetails::EXISTING_NODE)
+      url_spec = details_.existing_node->GetURL().spec();
+    gtk_entry_set_text(GTK_ENTRY(url_entry_), url_spec.c_str());
     g_signal_connect(G_OBJECT(url_entry_), "changed",
                      G_CALLBACK(OnEntryChanged), this);
     gtk_entry_set_activates_default(GTK_ENTRY(url_entry_), TRUE);
@@ -165,11 +173,14 @@ void BookmarkEditorGtk::Init(GtkWindow* parent_window) {
 
   if (show_tree_) {
     GtkTreeIter selected_iter;
-    int64 selected_id = parent_ ? parent_->id() : 0;
-    const BookmarkNode* node_to_ignore = IsEditingFolder() ? node_ : NULL;
+    int64 selected_id = 0;
+    if (details_.type == EditDetails::EXISTING_NODE)
+      selected_id = details_.existing_node->GetParent()->id();
+    else if (parent_)
+      selected_id = parent_->id();
     tree_store_ = bookmark_utils::MakeFolderTreeStore();
-    bookmark_utils::AddToTreeStore(bb_model_, selected_id, node_to_ignore,
-                                   tree_store_, &selected_iter);
+    bookmark_utils::AddToTreeStore(bb_model_, selected_id, tree_store_,
+                                   &selected_iter);
     tree_view_ = bookmark_utils::MakeTreeViewForStore(tree_store_);
     gtk_widget_set_size_request(tree_view_, kTreeWidth, kTreeHeight);
     tree_selection_ = gtk_tree_view_get_selection(GTK_TREE_VIEW(tree_view_));
@@ -246,7 +257,8 @@ void BookmarkEditorGtk::BookmarkNodeRemoved(BookmarkModel* model,
                                             const BookmarkNode* parent,
                                             int index,
                                             const BookmarkNode* node) {
-  if ((node_ && node_->HasAncestor(node)) ||
+  if ((details_.type == EditDetails::EXISTING_NODE &&
+       details_.existing_node->HasAncestor(node)) ||
       (parent_ && parent_->HasAncestor(node))) {
     // The node, or its parent was removed. Close the dialog.
     Close();
@@ -305,7 +317,7 @@ void BookmarkEditorGtk::ApplyEdits(GtkTreeIter* selected_parent) {
 
   if (!show_tree_ || !selected_parent) {
     bookmark_utils::ApplyEditsWithNoGroupChange(
-        bb_model_, parent_, node_, new_title, new_url, handler_.get());
+        bb_model_, parent_, details_, new_title, new_url, handler_.get());
     return;
   }
 
@@ -321,7 +333,7 @@ void BookmarkEditorGtk::ApplyEdits(GtkTreeIter* selected_parent) {
   }
 
   bookmark_utils::ApplyEditsWithPossibleGroupChange(
-      bb_model_, new_parent, node_, new_title, new_url, handler_.get());
+      bb_model_, new_parent, details_, new_title, new_url, handler_.get());
 }
 
 void BookmarkEditorGtk::AddNewGroup(GtkTreeIter* parent, GtkTreeIter* child) {
@@ -336,10 +348,6 @@ void BookmarkEditorGtk::AddNewGroup(GtkTreeIter* parent, GtkTreeIter* child) {
       -1);
 }
 
-bool BookmarkEditorGtk::IsEditingFolder() const {
-  return node_ && node_->is_folder();
-}
-
 // static
 void BookmarkEditorGtk::OnSelectionChanged(GtkTreeSelection* selection,
                                            BookmarkEditorGtk* dialog) {
@@ -352,9 +360,8 @@ void BookmarkEditorGtk::OnSelectionChanged(GtkTreeSelection* selection,
 // static
 void BookmarkEditorGtk::OnResponse(GtkDialog* dialog, int response_id,
                                    BookmarkEditorGtk* window) {
-  if (response_id == GTK_RESPONSE_ACCEPT) {
+  if (response_id == GTK_RESPONSE_ACCEPT)
     window->ApplyEdits();
-  }
 
   window->Close();
 }
@@ -382,9 +389,10 @@ void BookmarkEditorGtk::OnWindowDestroy(GtkWidget* widget,
 void BookmarkEditorGtk::OnEntryChanged(GtkEditable* entry,
                                        BookmarkEditorGtk* dialog) {
   gboolean can_close = TRUE;
-  if (dialog->IsEditingFolder()) {
+  if (dialog->details_.type == EditDetails::NEW_FOLDER) {
     if (dialog->GetInputTitle().empty()) {
-      gtk_widget_modify_base(dialog->name_entry_, GTK_STATE_NORMAL, &kErrorColor);
+      gtk_widget_modify_base(dialog->name_entry_, GTK_STATE_NORMAL,
+                             &kErrorColor);
       can_close = FALSE;
     } else {
       gtk_widget_modify_base(dialog->name_entry_, GTK_STATE_NORMAL, NULL);
@@ -392,7 +400,8 @@ void BookmarkEditorGtk::OnEntryChanged(GtkEditable* entry,
   } else {
     GURL url(dialog->GetInputURL());
     if (!url.is_valid()) {
-      gtk_widget_modify_base(dialog->url_entry_, GTK_STATE_NORMAL, &kErrorColor);
+      gtk_widget_modify_base(dialog->url_entry_, GTK_STATE_NORMAL,
+                             &kErrorColor);
       can_close = FALSE;
     } else {
       gtk_widget_modify_base(dialog->url_entry_, GTK_STATE_NORMAL, NULL);
diff --git a/chrome/browser/gtk/bookmark_editor_gtk.h b/chrome/browser/gtk/bookmark_editor_gtk.h
index 8d6efd9..d2ab6eb 100644
--- a/chrome/browser/gtk/bookmark_editor_gtk.h
+++ b/chrome/browser/gtk/bookmark_editor_gtk.h
@@ -33,7 +33,7 @@ class BookmarkEditorGtk : public BookmarkEditor,
   BookmarkEditorGtk(GtkWindow* window,
                     Profile* profile,
                     const BookmarkNode* parent,
-                    const BookmarkNode* node,
+                    const EditDetails& details,
                     BookmarkEditor::Configuration configuration,
                     BookmarkEditor::Handler* handler);
 
@@ -90,9 +90,6 @@ class BookmarkEditorGtk : public BookmarkEditor,
   // new group.
   void AddNewGroup(GtkTreeIter* parent, GtkTreeIter* child);
 
-  // Returns true if editing a folder.
-  bool IsEditingFolder() const;
-
   static void OnSelectionChanged(GtkTreeSelection* treeselection,
                                  BookmarkEditorGtk* dialog);
 
@@ -132,8 +129,8 @@ class BookmarkEditorGtk : public BookmarkEditor,
   // Initial parent to select. Is only used if node_ is NULL.
   const BookmarkNode* parent_;
 
-  // Node being edited. Is NULL if creating a new node.
-  const BookmarkNode* node_;
+  // Details about the node we're editing.
+  const EditDetails details_;
 
   // Mode used to create nodes from.
   BookmarkModel* bb_model_;
diff --git a/chrome/browser/gtk/bookmark_editor_gtk_unittest.cc b/chrome/browser/gtk/bookmark_editor_gtk_unittest.cc
index 7cb3a44..de03a51 100644
--- a/chrome/browser/gtk/bookmark_editor_gtk_unittest.cc
+++ b/chrome/browser/gtk/bookmark_editor_gtk_unittest.cc
@@ -88,7 +88,8 @@ class BookmarkEditorGtkTest : public testing::Test {
 
 // Makes sure the tree model matches that of the bookmark bar model.
 TEST_F(BookmarkEditorGtkTest, ModelsMatch) {
-  BookmarkEditorGtk editor(NULL, profile_.get(), NULL, NULL,
+  BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
+                           BookmarkEditor::EditDetails(),
                            BookmarkEditor::SHOW_TREE, NULL);
 
   // The root should have two children, one for the bookmark bar node,
@@ -127,7 +128,8 @@ TEST_F(BookmarkEditorGtkTest, ModelsMatch) {
 
 // Changes the title and makes sure parent/visual order doesn't change.
 TEST_F(BookmarkEditorGtkTest, EditTitleKeepsPosition) {
-  BookmarkEditorGtk editor(NULL, profile_.get(), NULL, GetNode("a"),
+  BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
+                           BookmarkEditor::EditDetails(GetNode("a")),
                            BookmarkEditor::SHOW_TREE, NULL);
   gtk_entry_set_text(GTK_ENTRY(editor.name_entry_), "new_a");
 
@@ -146,7 +148,8 @@ TEST_F(BookmarkEditorGtkTest, EditTitleKeepsPosition) {
 // Changes the url and makes sure parent/visual order doesn't change.
 TEST_F(BookmarkEditorGtkTest, EditURLKeepsPosition) {
   Time node_time = GetNode("a")->date_added();
-  BookmarkEditorGtk editor(NULL, profile_.get(), NULL, GetNode("a"),
+  BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
+                           BookmarkEditor::EditDetails(GetNode("a")),
                            BookmarkEditor::SHOW_TREE, NULL);
   gtk_entry_set_text(GTK_ENTRY(editor.url_entry_),
                      GURL(base_path() + "new_a").spec().c_str());
@@ -166,7 +169,8 @@ TEST_F(BookmarkEditorGtkTest, EditURLKeepsPosition) {
 
 // Moves 'a' to be a child of the other node.
 TEST_F(BookmarkEditorGtkTest, ChangeParent) {
-  BookmarkEditorGtk editor(NULL, profile_.get(), NULL, GetNode("a"),
+  BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
+                           BookmarkEditor::EditDetails(GetNode("a")),
                            BookmarkEditor::SHOW_TREE, NULL);
 
   GtkTreeModel* store = GTK_TREE_MODEL(editor.tree_store_);
@@ -184,7 +188,8 @@ TEST_F(BookmarkEditorGtkTest, ChangeParent) {
 // Moves 'a' to be a child of the other node and changes its url to new_a.
 TEST_F(BookmarkEditorGtkTest, ChangeParentAndURL) {
   Time node_time = GetNode("a")->date_added();
-  BookmarkEditorGtk editor(NULL, profile_.get(), NULL, GetNode("a"),
+  BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
+                           BookmarkEditor::EditDetails(GetNode("a")),
                            BookmarkEditor::SHOW_TREE, NULL);
 
   gtk_entry_set_text(GTK_ENTRY(editor.url_entry_),
@@ -204,7 +209,8 @@ TEST_F(BookmarkEditorGtkTest, ChangeParentAndURL) {
 
 // Creates a new folder and moves a node to it.
 TEST_F(BookmarkEditorGtkTest, MoveToNewParent) {
-  BookmarkEditorGtk editor(NULL, profile_.get(), NULL, GetNode("a"),
+  BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
+                           BookmarkEditor::EditDetails(GetNode("a")),
                            BookmarkEditor::SHOW_TREE, NULL);
 
   GtkTreeIter bookmark_bar_node;
@@ -251,7 +257,8 @@ TEST_F(BookmarkEditorGtkTest, MoveToNewParent) {
 
 // Brings up the editor, creating a new URL on the bookmark bar.
 TEST_F(BookmarkEditorGtkTest, NewURL) {
-  BookmarkEditorGtk editor(NULL, profile_.get(), NULL, NULL,
+  BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
+                           BookmarkEditor::EditDetails(),
                            BookmarkEditor::SHOW_TREE, NULL);
 
   gtk_entry_set_text(GTK_ENTRY(editor.url_entry_),
@@ -275,7 +282,8 @@ TEST_F(BookmarkEditorGtkTest, NewURL) {
 // Brings up the editor with no tree and modifies the url.
 TEST_F(BookmarkEditorGtkTest, ChangeURLNoTree) {
   BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
-                           model_->other_node()->GetChild(0),
+                           BookmarkEditor::EditDetails(
+                               model_->other_node()->GetChild(0)),
                            BookmarkEditor::NO_TREE, NULL);
 
   gtk_entry_set_text(GTK_ENTRY(editor.url_entry_),
@@ -296,7 +304,8 @@ TEST_F(BookmarkEditorGtkTest, ChangeURLNoTree) {
 // Brings up the editor with no tree and modifies only the title.
 TEST_F(BookmarkEditorGtkTest, ChangeTitleNoTree) {
   BookmarkEditorGtk editor(NULL, profile_.get(), NULL,
-                           model_->other_node()->GetChild(0),
+                           BookmarkEditor::EditDetails(
+                               model_->other_node()->GetChild(0)),
                            BookmarkEditor::NO_TREE, NULL);
   gtk_entry_set_text(GTK_ENTRY(editor.name_entry_), "new_a");
 
diff --git a/chrome/browser/gtk/bookmark_manager_gtk.cc b/chrome/browser/gtk/bookmark_manager_gtk.cc
index 96ae1ae..a1ad0d6 100644
--- a/chrome/browser/gtk/bookmark_manager_gtk.cc
+++ b/chrome/browser/gtk/bookmark_manager_gtk.cc
@@ -224,7 +224,7 @@ void BookmarkManagerGtk::BookmarkNodeAdded(BookmarkModel* model,
   if (node->is_folder()) {
     GtkTreeIter iter = { 0, };
     if (RecursiveFind(GTK_TREE_MODEL(left_store_), &iter, parent->id()))
-      bookmark_utils::AddToTreeStoreAt(node, 0, NULL, left_store_, NULL, &iter);
+      bookmark_utils::AddToTreeStoreAt(node, 0, left_store_, NULL, &iter);
   }
 }
 
@@ -615,7 +615,7 @@ void BookmarkManagerGtk::ResetOrganizeMenu(bool left) {
 void BookmarkManagerGtk::BuildLeftStore() {
   GtkTreeIter select_iter;
   bookmark_utils::AddToTreeStore(model_,
-      model_->GetBookmarkBarNode()->id(), NULL, left_store_, &select_iter);
+      model_->GetBookmarkBarNode()->id(), left_store_, &select_iter);
   gtk_tree_selection_select_iter(left_selection(), &select_iter);
 
   // TODO(estade): is there a decent stock icon we can use here?
diff --git a/chrome/browser/gtk/bookmark_tree_model.cc b/chrome/browser/gtk/bookmark_tree_model.cc
index e8e52ca..3c23740 100644
--- a/chrome/browser/gtk/bookmark_tree_model.cc
+++ b/chrome/browser/gtk/bookmark_tree_model.cc
@@ -98,12 +98,11 @@ GtkTreeStore* MakeFolderTreeStore() {
 }
 
 void AddToTreeStore(BookmarkModel* model, int64 selected_id,
-                    const BookmarkNode* node_to_ignore, GtkTreeStore* store,
-                    GtkTreeIter* selected_iter) {
+                    GtkTreeStore* store, GtkTreeIter* selected_iter) {
   const BookmarkNode* root_node = model->root_node();
   for (int i = 0; i < root_node->GetChildCount(); ++i) {
-    AddToTreeStoreAt(root_node->GetChild(i), selected_id, node_to_ignore, store,
-                     selected_iter, NULL);
+    AddToTreeStoreAt(root_node->GetChild(i), selected_id, store, selected_iter,
+                     NULL);
   }
 }
 
@@ -138,9 +137,9 @@ GtkCellRenderer* GetCellRendererText(GtkTreeView* tree_view) {
 }
 
 void AddToTreeStoreAt(const BookmarkNode* node, int64 selected_id,
-                      const BookmarkNode* node_to_ignore, GtkTreeStore* store,
-                      GtkTreeIter* selected_iter, GtkTreeIter* parent) {
-  if (!node->is_folder() || node == node_to_ignore)
+                      GtkTreeStore* store, GtkTreeIter* selected_iter,
+                      GtkTreeIter* parent) {
+  if (!node->is_folder())
     return;
 
   GtkTreeIter iter;
@@ -153,8 +152,8 @@ void AddToTreeStoreAt(const BookmarkNode* node, int64 selected_id,
   }
 
   for (int i = 0; i < node->GetChildCount(); ++i) {
-    AddToTreeStoreAt(node->GetChild(i), selected_id, node_to_ignore, store,
-                     selected_iter, &iter);
+    AddToTreeStoreAt(node->GetChild(i), selected_id, store, selected_iter,
+                     &iter);
   }
 }
 
diff --git a/chrome/browser/gtk/bookmark_tree_model.h b/chrome/browser/gtk/bookmark_tree_model.h
index 01da71f..80b1984 100644
--- a/chrome/browser/gtk/bookmark_tree_model.h
+++ b/chrome/browser/gtk/bookmark_tree_model.h
@@ -41,17 +41,14 @@ GtkTreeStore* MakeFolderTreeStore();
 // |recursive| indicates whether to recurse into sub-directories (if false,
 // the tree store will effectively be a list). |only_folders| indicates whether
 // to include bookmarks in the tree, or to only show folders.
-// If |node_to_ignore| is non-null, a node in the tree store is not created for
-// it.
 void AddToTreeStore(BookmarkModel* model, int64 selected_id,
-                    const BookmarkNode* node_to_ignore, GtkTreeStore* store,
-                    GtkTreeIter* selected_iter);
+                    GtkTreeStore* store, GtkTreeIter* selected_iter);
 
 // As above, but inserts just the tree rooted at |node| as a child of |parent|.
 // If |parent| is NULL, add it at the top level.
 void AddToTreeStoreAt(const BookmarkNode* node, int64 selected_id,
-                      const BookmarkNode* node_to_ignore, GtkTreeStore* store,
-                      GtkTreeIter* selected_iter, GtkTreeIter* parent);
+                      GtkTreeStore* store, GtkTreeIter* selected_iter,
+                      GtkTreeIter* parent);
 
 // Makes a tree view for the store. This will take ownership of |store| and the
 // returned widget has a floating reference.