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

4 files changed, 124 insertions, 66 deletions

diff --git a/chrome/browser/bookmarks/bookmark_context_menu_controller.cc b/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
index 15791a8..0e62277 100644
--- a/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
+++ b/chrome/browser/bookmarks/bookmark_context_menu_controller.cc
@@ -316,7 +316,8 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
           editor_config = BookmarkEditor::SHOW_TREE;
         else
           editor_config = BookmarkEditor::NO_TREE;
-        BookmarkEditor::Show(parent_window_, profile_, parent_, selection_[0],
+        BookmarkEditor::Show(parent_window_, profile_, parent_,
+                             BookmarkEditor::EditDetails(selection_[0]),
                              editor_config, NULL);
       } else {
         EditFolderController::Show(profile_, parent_window_, selection_[0],
@@ -349,7 +350,8 @@ void BookmarkContextMenuController::ExecuteCommand(int id) {
         handler = new SelectOnCreationHandler(profile_);
       }
       BookmarkEditor::Show(parent_window_, profile_, GetParentForNewNodes(),
-                           NULL, editor_config, handler);
+                           BookmarkEditor::EditDetails(), editor_config,
+                           handler);
       break;
     }
 
diff --git a/chrome/browser/bookmarks/bookmark_editor.h b/chrome/browser/bookmarks/bookmark_editor.h
index 0b98406..e935311 100644
--- a/chrome/browser/bookmarks/bookmark_editor.h
+++ b/chrome/browser/bookmarks/bookmark_editor.h
@@ -5,9 +5,14 @@
 #ifndef CHROME_BROWSER_BOOKMARKS_BOOKMARK_EDITOR_H_
 #define CHROME_BROWSER_BOOKMARKS_BOOKMARK_EDITOR_H_
 
+#include <string>
+#include <utility>
+#include <vector>
+
 #include "app/gfx/native_widget_types.h"
 
 class BookmarkNode;
+class GURL;
 class Profile;
 
 // Small, cross platform interface that shows the correct platform specific
@@ -28,20 +33,53 @@ class BookmarkEditor {
     NO_TREE
   };
 
-  // Shows the platform specific BookmarkEditor subclass editing |node|. |node|
-  // may be one of three values:
-  // . NULL, in which a case a new entry is created initially parented to
-  //  |parent|.
-  // . non-null and a url.
-  // . non-null and a folder. In this case the url field is not shown and an
-  //   entry for the node is not shown in the tree.
-  // If |show_tree| is false the tree is not shown. BookmarkEditor takes
-  // ownership of |handler| and deletes it when done. |handler| may be
-  // null. See description of Handler for details.
+  // Describes what the user is editing.
+  struct EditDetails {
+    enum Type {
+      // The user is editing an existing node in the model. The node the user
+      // is editing is set in |existing_node|.
+      EXISTING_NODE,
+
+      // A new bookmark should be created if the user accepts the edit.
+      // |existing_node| is null in this case.
+      NEW_URL,
+
+      // A new folder bookmark should be created if the user accepts the edit.
+      // The contents of the folder should be that of |urls|.
+      // |existing_node| is null in this case.
+      NEW_FOLDER
+    };
+
+    EditDetails() : type(NEW_URL), existing_node(NULL) {}
+
+    explicit EditDetails(const BookmarkNode* node)
+        : type(EXISTING_NODE),
+          existing_node(node) {
+    }
+
+    // See description of enum value for details.
+    Type type;
+
+    // If type == EXISTING_NODE this gives the existing node.
+    const BookmarkNode* existing_node;
+
+    // If type == NEW_FOLDER, this is the urls/title pairs to add to the
+    // folder.
+    std::vector<std::pair<GURL, std::wstring> > urls;
+  };
+
+  // Shows the bookmark editor. The bookmark editor allows editing an
+  // existing node or creating a new bookmark node (as determined by
+  // |details.type|). If |configuration| is SHOW_TREE, a tree is shown allowing
+  // the user to choose the parent of the node.
+  // |parent| gives the initial parent to select in the tree for the node.
+  // |parent| is only used if |details.existing_node| is null.
+  // BookmarkEditor takes ownership of |handler| and deletes it when done.
+  // |handler| may be null. See description of Handler for details.
   static void Show(gfx::NativeWindow parent_window,
                    Profile* profile,
                    const BookmarkNode* parent,
-                   const BookmarkNode* node,
+                   const EditDetails& details,
                    Configuration configuration,
                    Handler* handler);
 };
diff --git a/chrome/browser/bookmarks/bookmark_utils.cc b/chrome/browser/bookmarks/bookmark_utils.cc
index a8f5860..b11d0b9 100644
--- a/chrome/browser/bookmarks/bookmark_utils.cc
+++ b/chrome/browser/bookmarks/bookmark_utils.cc
@@ -505,22 +505,44 @@ bool DoesBookmarkContainText(const BookmarkNode* node,
   return (node->is_url() && DoesBookmarkContainWords(node, words, languages));
 }
 
-const BookmarkNode* ApplyEditsWithNoGroupChange(BookmarkModel* model,
-    const BookmarkNode* parent, const BookmarkNode* node,
+static const BookmarkNode* CreateNewNode(BookmarkModel* model,
+    const BookmarkNode* parent, const BookmarkEditor::EditDetails& details,
     const std::wstring& new_title, const GURL& new_url,
     BookmarkEditor::Handler* handler) {
-  const BookmarkNode* old_parent = node ? node->GetParent() : NULL;
-  const int old_index = old_parent ? old_parent->IndexOfChild(node) : -1;
+  const BookmarkNode* node;
+  if (details.type == BookmarkEditor::EditDetails::NEW_URL) {
+    node = model->AddURL(parent, parent->GetChildCount(), new_title, new_url);
+  } else if (details.type == BookmarkEditor::EditDetails::NEW_FOLDER) {
+    node = model->AddGroup(parent, parent->GetChildCount(), new_title);
+    for (size_t i = 0; i < details.urls.size(); ++i) {
+      model->AddURL(node, node->GetChildCount(), details.urls[i].second,
+                    details.urls[i].first);
+    }
+    // TODO(sky): update parent modified time.
+  } else {
+    NOTREACHED();
+    return NULL;
+  }
 
-  if (!node) {
-    node =
-        model->AddURL(parent, parent->GetChildCount(), new_title, new_url);
+  if (handler)
+    handler->NodeCreated(node);
+  return node;
+}
 
-    if (handler)
-      handler->NodeCreated(node);
-    return node;
+const BookmarkNode* ApplyEditsWithNoGroupChange(BookmarkModel* model,
+    const BookmarkNode* parent, const BookmarkEditor::EditDetails& details,
+    const std::wstring& new_title, const GURL& new_url,
+    BookmarkEditor::Handler* handler) {
+  if (details.type == BookmarkEditor::EditDetails::NEW_URL ||
+      details.type == BookmarkEditor::EditDetails::NEW_FOLDER) {
+    return CreateNewNode(model, parent, details, new_title, new_url, handler);
   }
 
+  const BookmarkNode* node = details.existing_node;
+  DCHECK(node);
+  const BookmarkNode* old_parent = node->GetParent();
+  int old_index = old_parent ? old_parent->IndexOfChild(node) : -1;
+
   // If we're not showing the tree we only need to modify the node.
   if (old_index == -1) {
     NOTREACHED();
@@ -528,6 +550,7 @@ const BookmarkNode* ApplyEditsWithNoGroupChange(BookmarkModel* model,
   }
 
   if (new_url != node->GetURL()) {
+    // TODO(sky): need SetURL on the model.
     const BookmarkNode* new_node = model->AddURLWithCreationTime(old_parent,
         old_index, new_title, new_url, node->date_added());
     model->Remove(old_parent, old_index + 1);
@@ -539,40 +562,40 @@ const BookmarkNode* ApplyEditsWithNoGroupChange(BookmarkModel* model,
 }
 
 const BookmarkNode* ApplyEditsWithPossibleGroupChange(BookmarkModel* model,
-    const BookmarkNode* new_parent, const BookmarkNode* node,
+    const BookmarkNode* new_parent, const BookmarkEditor::EditDetails& details,
     const std::wstring& new_title, const GURL& new_url,
     BookmarkEditor::Handler* handler) {
-  const BookmarkNode* old_parent = node ? node->GetParent() : NULL;
-  const int old_index = old_parent ? old_parent->IndexOfChild(node) : -1;
+  if (details.type == BookmarkEditor::EditDetails::NEW_URL ||
+      details.type == BookmarkEditor::EditDetails::NEW_FOLDER) {
+    return CreateNewNode(model, new_parent, details, new_title, new_url,
+                         handler);
+  }
+
+  const BookmarkNode* node = details.existing_node;
+  DCHECK(node);
+  const BookmarkNode* old_parent = node->GetParent();
+  int old_index = old_parent->IndexOfChild(node);
   const BookmarkNode* return_node = node;
-  if (node) {
-    Time date_added = node->date_added();
-    if (new_parent == node->GetParent()) {
-      // The parent is the same.
-      if (node->is_url() && new_url != node->GetURL()) {
-        model->Remove(old_parent, old_index);
-        return_node = model->AddURLWithCreationTime(old_parent, old_index,
-            new_title, new_url, date_added);
-      } else {
-        model->SetTitle(node, new_title);
-      }
-    } else if (node->is_url() && new_url != node->GetURL()) {
-      // The parent and URL changed.
+
+  Time date_added = node->date_added();
+  if (new_parent == node->GetParent()) {
+    // The parent is the same.
+    if (node->is_url() && new_url != node->GetURL()) {
       model->Remove(old_parent, old_index);
-      return_node = model->AddURLWithCreationTime(new_parent,
-          new_parent->GetChildCount(), new_title, new_url, date_added);
+      return_node = model->AddURLWithCreationTime(old_parent, old_index,
+          new_title, new_url, date_added);
     } else {
-      // The parent and title changed. Move the node and change the title.
-      model->Move(node, new_parent, new_parent->GetChildCount());
       model->SetTitle(node, new_title);
     }
+  } else if (node->is_url() && new_url != node->GetURL()) {
+    // The parent and URL changed.
+    model->Remove(old_parent, old_index);
+    return_node = model->AddURLWithCreationTime(new_parent,
+        new_parent->GetChildCount(), new_title, new_url, date_added);
   } else {
-    // We're adding a new URL.
-    return_node =
-        model->AddURL(new_parent, new_parent->GetChildCount(), new_title,
-                      new_url);
-    if (handler)
-      handler->NodeCreated(return_node);
+    // The parent and title changed. Move the node and change the title.
+    model->Move(node, new_parent, new_parent->GetChildCount());
+    model->SetTitle(node, new_title);
   }
   return return_node;
 }
@@ -618,20 +641,14 @@ void GetURLAndTitleToBookmark(TabContents* tab_contents,
   *title = UTF16ToWideHack(tab_contents->GetTitle());
 }
 
-const BookmarkNode* CreateBookmarkForAllTabs(Browser* browser) {
-  BookmarkModel* model = browser->profile()->GetBookmarkModel();
-  DCHECK(model && model->IsLoaded());
-  const BookmarkNode* parent = model->GetParentForNewNodes();
-  const BookmarkNode* folder = model->AddGroup(
-      parent, parent->GetChildCount(),
-      l10n_util::GetString(IDS_BOOMARK_EDITOR_NEW_FOLDER_NAME));
+void GetURLsForOpenTabs(Browser* browser,
+    std::vector<std::pair<GURL, std::wstring> >* urls) {
   for (int i = 0; i < browser->tab_count(); ++i) {
-    GURL url;
-    std::wstring title;
-    GetURLAndTitleToBookmark(browser->GetTabContentsAt(i), &url, &title);
-    model->AddURL(folder, folder->GetChildCount(), title, url);
+    std::pair<GURL, std::wstring> entry;
+    GetURLAndTitleToBookmark(browser->GetTabContentsAt(i), &(entry.first),
+                             &(entry.second));
+    urls->push_back(entry);
   }
-  return folder;
 }
 
 }  // namespace bookmark_utils
diff --git a/chrome/browser/bookmarks/bookmark_utils.h b/chrome/browser/bookmarks/bookmark_utils.h
index 0c5d53d..7117967 100644
--- a/chrome/browser/bookmarks/bookmark_utils.h
+++ b/chrome/browser/bookmarks/bookmark_utils.h
@@ -155,7 +155,7 @@ bool DoesBookmarkContainText(const BookmarkNode* node,
 const BookmarkNode* ApplyEditsWithNoGroupChange(
     BookmarkModel* model,
     const BookmarkNode* parent,
-    const BookmarkNode* node,
+    const BookmarkEditor::EditDetails& details,
     const std::wstring& new_title,
     const GURL& new_url,
     BookmarkEditor::Handler* handler);
@@ -167,7 +167,7 @@ const BookmarkNode* ApplyEditsWithNoGroupChange(
 const BookmarkNode* ApplyEditsWithPossibleGroupChange(
     BookmarkModel* model,
     const BookmarkNode* new_parent,
-    const BookmarkNode* node,
+    const BookmarkEditor::EditDetails& details,
     const std::wstring& new_title,
     const GURL& new_url,
     BookmarkEditor::Handler* handler);
@@ -187,9 +187,10 @@ void GetURLAndTitleToBookmark(TabContents* tab_contents,
                               GURL* url,
                               std::wstring* title);
 
-// Creates a new folder containing a bookmark for each of the tabs in
-// |browser|.
-const BookmarkNode* CreateBookmarkForAllTabs(Browser* browser);
+// Returns, by reference in |urls|, the url and title pairs for each open
+// tab in browser.
+void GetURLsForOpenTabs(Browser* browser,
+                        std::vector<std::pair<GURL, std::wstring> >* urls);
 
 // Number of bookmarks we'll open before prompting the user to see if they
 // really want to open all.