bookmark STAR bubble: Disambiguate folders with the same name

BUG=http://crbug.com/19408

TEST=Create a bookmark.
Create 2 bookmark folders both with the same name, "foo".
Go to your bookmarked page.
Click STAR to get bookmark bubble.
Change parent folder to the 1st "foo".
Confirm it's there on the bar.
Click STAR to get bookmark bubble.
Change parent folder to the 2nd "foo".
Confirm it's there on the bar.
Click STAR to get bookmark bubble.
Chose "choose another folder" to be sure that logic still works.

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

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

3 files changed, 67 insertions, 20 deletions

diff --git a/chrome/browser/cocoa/bookmark_bubble_controller.h b/chrome/browser/cocoa/bookmark_bubble_controller.h
index 0ad77c1..9aba12f 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller.h
+++ b/chrome/browser/cocoa/bookmark_bubble_controller.h
@@ -32,8 +32,8 @@ class BookmarkNode;
   BookmarkModel* model_;  // weak
   const BookmarkNode* node_;  // weak
 
-  // A mapping from titles to nodes so we only have to walk this once.
-  scoped_nsobject<NSMutableArray> titleMapping_;
+  // A mapping from NSComboBox index to parent nodes.
+  scoped_nsobject<NSMutableArray> parentMapping_;
 
   BOOL alreadyBookmarked_;
   scoped_nsobject<NSString> chooseAnotherFolder_;
@@ -71,6 +71,7 @@ class BookmarkNode;
 - (void)addFolderNodes:(const BookmarkNode*)parent toComboBox:(NSComboBox*)box;
 - (void)updateBookmarkNode;
 - (void)setTitle:(NSString*)title parentFolder:(NSString*)folder;
+- (void)setParentFolderSelection:(const BookmarkNode*)parent;
 - (NSString*)chooseAnotherFolderString;
 - (NSComboBox*)folderComboBox;
 @end
diff --git a/chrome/browser/cocoa/bookmark_bubble_controller.mm b/chrome/browser/cocoa/bookmark_bubble_controller.mm
index cb88fd5..5e1d7f1 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller.mm
+++ b/chrome/browser/cocoa/bookmark_bubble_controller.mm
@@ -30,7 +30,7 @@
     node_ = node;
     alreadyBookmarked_ = alreadyBookmarked;
     // But this is strong.
-    titleMapping_.reset([[NSMutableDictionary alloc] init]);
+    parentMapping_.reset([[NSMutableArray alloc] init]);
   }
   return self;
 }
@@ -126,12 +126,10 @@
 
 
 // Fill in all information related to the folder combo box.
-//
-// TODO(jrg): make sure nested folders that have the same name are
-// handled properly.
-// http://crbug.com/19408
 - (void)fillInFolderList {
   [nameTextField_ setStringValue:base::SysWideToNSString(node_->GetTitle())];
+  DCHECK([parentMapping_ count] == 0);
+  DCHECK([folderComboBox_ numberOfItems] == 0);
   [self addFolderNodes:model_->root_node() toComboBox:folderComboBox_];
 
   // Add "Choose another folder...".  Remember it for later to compare against.
@@ -148,13 +146,12 @@
 
 // For the given folder node, walk the tree and add folder names to
 // the given combo box.
-//
-// TODO(jrg): no distinction is made among folders with the same name.
 - (void)addFolderNodes:(const BookmarkNode*)parent toComboBox:(NSComboBox*)box {
   NSString* title = base::SysWideToNSString(parent->GetTitle());
   if ([title length])  {  // no title if root
     [box addItemWithObjectValue:title];
-    [titleMapping_ setValue:[NSValue valueWithPointer:parent] forKey:title];
+    [parentMapping_ insertObject:[NSValue valueWithPointer:parent]
+                         atIndex:[box numberOfItems]-1];
   }
   for (int i = 0; i < parent->GetChildCount(); i++) {
     const BookmarkNode* child = parent->GetChild(i);
@@ -177,19 +174,23 @@
                               model_->profile());
   }
   // Then the parent folder.
-  NSString* oldParentTitle = base::SysWideToNSString(
-    node_->GetParent()->GetTitle());
-  NSString* newParentTitle = [folderComboBox_ objectValueOfSelectedItem];
-  if (![oldParentTitle isEqual:newParentTitle]) {
-    const BookmarkNode* newParent = static_cast<const BookmarkNode*>(
-      [[titleMapping_ objectForKey:newParentTitle] pointerValue]);
-    if (newParent) {
-      // newParent should only ever possibly be NULL in a unit test.
+  const BookmarkNode* oldParent = node_->GetParent();
+  NSInteger selectedIndex = [folderComboBox_ indexOfSelectedItem];
+  if (selectedIndex == -1)  // No selection ever made.
+    return;
+
+  if ((NSUInteger)selectedIndex == [parentMapping_ count]) {
+    // "Choose another folder..."
+    return;
+  }
+  const BookmarkNode* newParent = static_cast<const BookmarkNode*>(
+      [[parentMapping_ objectAtIndex:selectedIndex] pointerValue]);
+  DCHECK(newParent);
+  if (oldParent != newParent) {
       int index = newParent->GetChildCount();
       model_->Move(node_, newParent, index);
       UserMetrics::RecordAction(L"BookmarkBubble_ChangeParent",
                                 model_->profile());
-    }
   }
 }
 
@@ -198,6 +199,25 @@
   [folderComboBox_ selectItemWithObjectValue:folder];
 }
 
+// Pick a specific parent node in the selection by finding the right
+// combo box index.
+- (void)setParentFolderSelection:(const BookmarkNode*)parent {
+  // Expectation: we have a parent mapping for all items in the
+  // folderComboBox except the last one ("Choose another folder...").
+  DCHECK((NSInteger)[parentMapping_ count] ==
+         [folderComboBox_ numberOfItems]-1);
+  for (NSUInteger i = 0; i < [parentMapping_ count]; i++) {
+    const BookmarkNode* possible = static_cast<const BookmarkNode*>(
+        [[parentMapping_ objectAtIndex:i] pointerValue]);
+    DCHECK(possible);
+    if (possible == parent) {
+      [folderComboBox_ selectItemAtIndex:i];
+      return;
+    }
+  }
+  NOTREACHED();
+}
+
 - (NSString*)chooseAnotherFolderString {
   return chooseAnotherFolder_.get();
 }
diff --git a/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm b/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
index b090504..d632d92 100644
--- a/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm
@@ -79,7 +79,8 @@ class BookmarkBubbleControllerTest : public CocoaTest {
   // Returns a controller but ownership not transferred.
   // Only one of these will be valid at a time.
   BookmarkBubbleController* ControllerForNode(const BookmarkNode* node) {
-    DCHECK(controller_ == nil);
+    if (controller_)
+      [controller_ close];
     controller_ = [[BookmarkBubbleController alloc]
                        initWithDelegate:delegate_.get()
                        parentWindow:test_window()
@@ -196,6 +197,31 @@ TEST_F(BookmarkBubbleControllerTest, TestUserEdit) {
   EXPECT_EQ(node->GetParent()->GetTitle(), L"grandma");
 }
 
+// Confirm happiness with parent nodes that have the same name.
+TEST_F(BookmarkBubbleControllerTest, TestNewParentSameName) {
+  for (int i=0; i<2; i++) {
+    BookmarkModel* model = GetBookmarkModel();
+    const BookmarkNode* node = model->AddURL(model->GetBookmarkBarNode(),
+                                             0,
+                                             L"short-title",
+                                             GURL("http://www.google.com"));
+    model->AddGroup(model->GetBookmarkBarNode(), 0, L"NAME");
+    model->AddGroup(model->GetBookmarkBarNode(), 0, L"NAME");
+    model->AddGroup(model->GetBookmarkBarNode(), 0, L"NAME");
+    BookmarkBubbleController* controller = ControllerForNode(node);
+    EXPECT_TRUE(controller);
+
+    // simulate a user edit
+    [controller setParentFolderSelection:
+                  model->GetBookmarkBarNode()->GetChild(i)];
+    [controller edit:controller];
+
+    // Make sure bookmark has changed, and that the parent is what we
+    // expect.  This proves nobody did searching based on name.
+    EXPECT_EQ(node->GetParent(), model->GetBookmarkBarNode()->GetChild(i));
+  }
+}
+
 // Click the "remove" button
 TEST_F(BookmarkBubbleControllerTest, TestRemove) {
   BookmarkModel* model = GetBookmarkModel();