Implement bookmark editor (no tree yet)

Implement bookmark editor.  No tree display or hierarchy movement, but
name/url editing works.  Get to the edotir from a context menu (Edit,
Add Page).  Also Implement Open All Bookmarks menu item.

BUG=http://crbug.com/8381, http://crbug.com/17006 

TEST=Add some bookmarks.  
Right-click on a bookmark and pick Edit.
Test editing the name and URL.  Make sure you can't add a bogus URL.
Right-click on a bookmark or the bar and Add Page.
Fill in name and URL fields to add a new bookmark.
Right-click Open All Bookmarks and make sure it hoses your machine.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=21241

[John Grabowski, 2009-07-21 20:53:38]

Rohit: please review.
tvl: please direct as appropriate for l10n issues if there are new rules to
follow.
+cc pink because i know he's itching for this

[TVL, 2009-07-21 21:22:00]

just make sure the UI is as much like windows and I'll probably use it as one of
the examples as i finish this up.  :)

[rohitrao (ping after 24h), 2009-07-21 22:06:14]

LGTM, with some nits and questions.

http://codereview.chromium.org/155874/diff/1/7
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/155874/diff/1/7#newcode86
Line 86: // Actions for manipulatying bookmarks.
Typo: manupulatying

http://codereview.chromium.org/155874/diff/1/6
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/155874/diff/1/6#newcode13
Line 13: @interface BookmarkEditorController(Private)
This is just the popup bubble, right?  Not the full bookmark manager?

http://codereview.chromium.org/155874/diff/1/6#newcode63
Line 63: if (node_) {
This scares me.  I'm worried that I could add a [self view] call in early in
init that would invoke awakeFromNib before node_ is set.

http://codereview.chromium.org/155874/diff/1/6#newcode66
Line 66: initialUrl_.reset([[NSString stringWithUTF8String:url_string.c_str()]
retain]);
80 chars.

http://codereview.chromium.org/155874/diff/1/6#newcode78
Line 78: // Remember the browser's height; we will shrink our frame by that
I got really confused by the use of browser and window here, until I realized
that you're talking about an NSBrowser and the child window.  Changing
"browser's" to "NSBrowser's" may help.

http://codereview.chromium.org/155874/diff/1/6#newcode173
Line 173: return [nameField_ setStringValue:name];
No return.

http://codereview.chromium.org/155874/diff/1/6#newcode177
Line 177: return [urlField_ setStringValue:name];
No return.

http://codereview.chromium.org/155874/diff/1/5
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/155874/diff/1/5#newcode66
Line 66: const char* url_name = "http://www.zim-bop-a-doo.com/";
Should be "http://www.zim-bop-a-dee.com/"

http://codereview.chromium.org/155874/diff/1/5#newcode89
Line 89: [controller ok:nil];
Why can you continue to interact with the controller after calling ok:?  Doesn't
ok: autorelease the controller?

[John Grabowski, 2009-07-21 23:30:44]

Thanks Rohit.
tvl, the few strings in here are just like Windows (e.g. "OK" on the button).

http://codereview.chromium.org/155874/diff/1/7
File chrome/browser/cocoa/bookmark_bar_controller.h (right):

http://codereview.chromium.org/155874/diff/1/7#newcode86
Line 86: // Actions for manipulatying bookmarks.
On 2009/07/21 22:06:14, rohitrao wrote:
> Typo: manupulatying

fyxed

http://codereview.chromium.org/155874/diff/1/6
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/155874/diff/1/6#newcode13
Line 13: @interface BookmarkEditorController(Private)
On 2009/07/21 22:06:14, rohitrao wrote:
> This is just the popup bubble, right?  Not the full bookmark manager?

This is NOT the full manager.

There are actually 2 kinds of popups.  One is the bubble on create bookmark with
Cmd-D.  That's not this.

The other is when editing a bookmark (context menu over button --> Edit) or
adding a bookmark from a context menu over the bar or button (menu --> Add
Page).   That's what this is.

http://codereview.chromium.org/155874/diff/1/6#newcode63
Line 63: if (node_) {
On 2009/07/21 22:06:14, rohitrao wrote:
> This scares me.  I'm worried that I could add a [self view] call in early in
> init that would invoke awakeFromNib before node_ is set.

I'm not sure how I could prevent that.  Using an object from within it's init
routine, somewhat by definition, means that initialization hasn't finished.  Is
there something you can suggest?

http://codereview.chromium.org/155874/diff/1/6#newcode66
Line 66: initialUrl_.reset([[NSString stringWithUTF8String:url_string.c_str()]
retain]);
On 2009/07/21 22:06:14, rohitrao wrote:
> 80 chars.

fixed

http://codereview.chromium.org/155874/diff/1/6#newcode78
Line 78: // Remember the browser's height; we will shrink our frame by that
On 2009/07/21 22:06:14, rohitrao wrote:
> I got really confused by the use of browser and window here, until I realized
> that you're talking about an NSBrowser and the child window.  Changing
> "browser's" to "NSBrowser's" may help.

Clarified

http://codereview.chromium.org/155874/diff/1/6#newcode173
Line 173: return [nameField_ setStringValue:name];
On 2009/07/21 22:06:14, rohitrao wrote:
> No return.

removed

http://codereview.chromium.org/155874/diff/1/6#newcode177
Line 177: return [urlField_ setStringValue:name];
On 2009/07/21 22:06:14, rohitrao wrote:
> No return.

removed

http://codereview.chromium.org/155874/diff/1/5
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/155874/diff/1/5#newcode66
Line 66: const char* url_name = "http://www.zim-bop-a-doo.com/";
On 2009/07/21 22:06:14, rohitrao wrote:
> Should be "http://www.zim-bop-a-dee.com/"

Doh!  Fixed (in entire file)

http://codereview.chromium.org/155874/diff/1/5#newcode89
Line 89: [controller ok:nil];
On 2009/07/21 22:06:14, rohitrao wrote:
> Why can you continue to interact with the controller after calling ok:? 
Doesn't
> ok: autorelease the controller?

No; I never beginSheet: on this window, so endSheet: has nothing to do (and thus
didEndSheet::: never gets called to autorelease it).