Mac bookmark manager: Make URLs editable.

BUG=32028
TEST=BookmarkItemTest.URL*. Manually: click the URL of a selected item and it will become editable. Type new value and hit Return/Enter.

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

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

6 files changed, 80 insertions, 12 deletions

diff --git a/chrome/browser/cocoa/bookmark_item.mm b/chrome/browser/cocoa/bookmark_item.mm
index d66ded4..f59022a 100644
--- a/chrome/browser/cocoa/bookmark_item.mm
+++ b/chrome/browser/cocoa/bookmark_item.mm
@@ -17,6 +17,20 @@
 #include "skia/ext/skia_utils_mac.h"
 
 
+static GURL ConvertToGURL(NSString* urlStr) {
+  DCHECK(urlStr);
+  GURL url(base::SysNSStringToUTF8(urlStr));
+  if (!url.is_valid()) {
+    // Mimic observed friendliness from Windows
+    urlStr = [@"http://" stringByAppendingString:urlStr];
+    GURL fixedURL(base::SysNSStringToUTF8(urlStr));
+    if (fixedURL.is_valid())
+      url = fixedURL;
+  }
+  return url;
+}
+
+
 @implementation BookmarkItem
 
 @synthesize node = node_;
@@ -53,10 +67,23 @@
 }
 
 - (void)setURLString:(NSString*)urlStr {
-  //TODO(snej): Uncomment this once SetURL exists (bug 10603).
-  //  ...or work around it by removing node and adding new one.
-  //[manager_ bookmarkModel]->SetURL(node_, GURL([urlStr UTF8String]));
+  DCHECK(![self isFolder]);
+  DCHECK(![self isFixed]);
+  GURL url = ConvertToGURL(urlStr);
+  if (url == node_->GetURL() || !url.is_valid())
+    return;
+
   icon_.reset(nil);
+  // Create a new node with the updated URL.
+  BookmarkModel* model = [manager_ bookmarkModel];
+  const BookmarkNode* parent = node_->GetParent();
+  int index = parent->IndexOfChild(node_);
+  node_ = model->AddURLWithCreationTime(parent, index, node_->GetTitle(),
+                                        url, node_->date_added());
+  // Tell the manager that the new node maps to me.
+  [manager_ remapItem:self forNode:node_];
+  // Remove my old node.
+  model->Remove(parent, index + 1);
 }
 
 - (BookmarkItem*)parent {
@@ -200,6 +227,8 @@
   if (!node_ || !node_->is_url())
     return NO;
   GURL url = node_->GetURL();
+  if (url.is_empty())
+    return NO;
 
   Browser* browser = BrowserList::GetLastActive();
   // if no browser window exists then create one with no tabs to be filled in
@@ -227,11 +256,14 @@
   DCHECK_GT([urlString length], 0U);
   DCHECK([self isFolder]);
   DCHECK(node_);
+  GURL url = ConvertToGURL(urlString);
+  if (!url.is_valid())
+    return nil;
   BookmarkModel* model = [manager_ bookmarkModel];
   const BookmarkNode* node = model->AddURL([self node],
                                            index,
                                            base::SysNSStringToWide(title),
-                                           GURL([urlString UTF8String]));
+                                           url);
   return [manager_ itemFromNode:node];
 }
 
diff --git a/chrome/browser/cocoa/bookmark_item_unittest.mm b/chrome/browser/cocoa/bookmark_item_unittest.mm
index a2826de..64ca3c4 100644
--- a/chrome/browser/cocoa/bookmark_item_unittest.mm
+++ b/chrome/browser/cocoa/bookmark_item_unittest.mm
@@ -162,6 +162,29 @@ TEST_F(BookmarkItemTest, AddItems) {
   EXPECT_EQ(0U, [folder indexOfChild:item2]);
 }
 
+TEST_F(BookmarkItemTest, URLEditing) {
+  BookmarkItem* item = [bar_ addBookmarkWithTitle:@"URL"
+                                              URL:@"http://www.google.com/"
+                                          atIndex:0];
+  EXPECT_TRUE(item);
+  EXPECT_TRUE([@"http://www.google.com/" isEqual:[item URLString]]);
+  [item setURLString: @"http://www.google.com/chrome"];
+  EXPECT_TRUE([@"http://www.google.com/chrome" isEqual:[item URLString]]);
+}
+
+TEST_F(BookmarkItemTest, URLConversion) {
+  BookmarkItem* item = [bar_ addBookmarkWithTitle:@"fixable URL"
+                                              URL:@"www.google.com"
+                                          atIndex:0];
+  EXPECT_TRUE(item);
+  EXPECT_TRUE([@"http://www.google.com/" isEqual:[item URLString]]);
+
+  item = [bar_ addBookmarkWithTitle:@"bad URL"
+                                URL:@"!$%@djd ^%QQQ"
+                            atIndex:0];
+  EXPECT_FALSE(item);
+}
+
 TEST_F(BookmarkItemTest, Hierarchy) {
   AddTestBookmarks();
   ASSERT_EQ(4U, [bar_ numberOfChildren]);
diff --git a/chrome/browser/cocoa/bookmark_manager_controller.h b/chrome/browser/cocoa/bookmark_manager_controller.h
index 6101419..9c33b27 100644
--- a/chrome/browser/cocoa/bookmark_manager_controller.h
+++ b/chrome/browser/cocoa/bookmark_manager_controller.h
@@ -58,6 +58,8 @@ class Profile;
 // Called by the toolbar item; forwards to the focused tree controller.
 - (IBAction)newFolder:(id)sender;
 
+// Updates the node->item mapping; called only by BookmarkItem itself!
+- (void)remapItem:(BookmarkItem*)item forNode:(const BookmarkNode*)node;
 @end
 
 
diff --git a/chrome/browser/cocoa/bookmark_manager_controller.mm b/chrome/browser/cocoa/bookmark_manager_controller.mm
index b5f6063..50708a5 100644
--- a/chrome/browser/cocoa/bookmark_manager_controller.mm
+++ b/chrome/browser/cocoa/bookmark_manager_controller.mm
@@ -259,6 +259,11 @@ class BookmarkManagerBridge : public BookmarkModelObserver {
   return [self itemFromNode:[self bookmarkModel]->other_node()];
 }
 
+// Updates the mapping; called by a BookmarkItem if it changes its node.
+- (void)remapItem:(BookmarkItem*)item forNode:(const BookmarkNode*)node {
+  NSMapInsert(nodeMap_, node, item);
+}
+
 // Removes a BookmarkNode from the node<->item mapping table.
 - (void)forgetNode:(const BookmarkNode*)node {
   NSMapRemove(nodeMap_, node);
diff --git a/chrome/browser/cocoa/bookmark_tree_controller.mm b/chrome/browser/cocoa/bookmark_tree_controller.mm
index 60d4ad7..704f8ac 100644
--- a/chrome/browser/cocoa/bookmark_tree_controller.mm
+++ b/chrome/browser/cocoa/bookmark_tree_controller.mm
@@ -400,7 +400,8 @@ static void addItem(NSMenu* menu, int command, SEL action) {
   if ([ident isEqualToString:kTitleColIdent]) {
     [item setTitle:value];
   } else if ([ident isEqualToString:kURLColIdent]) {
-    [item setURLString:value];
+    if ([value length])
+      [item setURLString:value];
   }
 }
 
@@ -408,9 +409,12 @@ static void addItem(NSMenu* menu, int command, SEL action) {
 - (BOOL)      outlineView:(NSOutlineView*)outlineView
     shouldEditTableColumn:(NSTableColumn*)tableColumn
                      item:(id)item {
-  //TODO(snej): Make URL column editable once setter method exists (bug 10603).
   NSString* ident = [tableColumn identifier];
-  return [ident isEqualToString:kTitleColIdent] && ![item isFixed];
+  if ([item isFixed])
+    return NO;
+  if ([ident isEqualToString:kURLColIdent] && [item isFolder])
+    return NO;
+  return YES;
 }
 
 // Sets a cell's icon before it's drawn (NSOutlineView data source)
diff --git a/chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm b/chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm
index fb6e290..88cc27e 100644
--- a/chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm
+++ b/chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm
@@ -295,10 +295,12 @@ static NSDictionary* makeBookmarkPlistEntry(NSString* name, NSString* urlStr) {
                          objectForKey:kURIDictTitleKey];
       NSString* urlStr = [plistItem objectForKey:kURLStringKey];
       if (title && urlStr) {
-        [dstParent addBookmarkWithTitle:title
-                                    URL:urlStr
-                                atIndex:dstIndex + i];
-        ++i;
+        BookmarkItem* newItem = [dstParent addBookmarkWithTitle:title
+                                                            URL:urlStr
+                                                        atIndex:dstIndex + i];
+        // It is possible for that call to fail if urlStr isn't a valid URL.
+        if (newItem)
+          ++i;
       }
     } else {
       NSString* title = [plistItem objectForKey:kTitleKey];
@@ -315,7 +317,7 @@ static NSDictionary* makeBookmarkPlistEntry(NSString* name, NSString* urlStr) {
     [pool drain];
   }
   [self selectItemsInFolder:dstParent
-                  atIndexes:NSMakeRange(dstIndex, [plist count])];
+                  atIndexes:NSMakeRange(dstIndex, i)];
   return YES;
 }