Bookmark STAR bubble

Bookmark STAR bubble.

BUG=http://crbug.com/14929
Sample image attached to bug.

TEST=Click the STAR to add a bookmark.
Watch bubble come up.  Title is "Bookmark added!"
Confirm fields are OK.
Switch tabs and see bubble go away.
Click STAR again.
Watch bubble come up.  Title is "Bookmark"
Make sure all the buttons work (Edit, Close, Remove).
Make sure you can change the title and parent folder.
Make sure "Choose another folder..." opens edit window.


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

[John Grabowski, 2009-08-15 02:34:13]

+cc tvl to confirm l10n done properly

[TVL, 2009-08-17 01:23:18]

If i remember this ui right, we'll need the layout tweaking support in here also
to handle some localizations.

http://codereview.chromium.org/171016/diff/1003/1010
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/171016/diff/1003/1010#newcode5
Line 5: #include "app/l10n_util.h"
use l10n_util_mac?

http://codereview.chromium.org/171016/diff/1003/1010#newcode53
Line 53: l10n_util::GetString(IDS_BOOMARK_BUBBLE_PAGE_BOOKMARKED));
use l10n_util:GetNSString?

http://codereview.chromium.org/171016/diff/1003/1010#newcode106
Line 106: retain]);
l10n_util::GetNSString?

Do you need worry about windows accelerator markers or ellipsis? 
(l10n_util::FixUpWindowsStyleLabel)

http://codereview.chromium.org/171016/diff/1003/1010#newcode218
Line 218: // - If I @synthesize them in the category, it yells that it isn't
allowed.
can you do them in a @interface BookmarkBubbleController () decl?  objc2 added
that for declaring methods have having the @impl checks for them (to support
privates) so it might work on properties too.

[pink (ping after 24hrs), 2009-08-17 17:06:46]

http://codereview.chromium.org/171016/diff/1003/1025
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/171016/diff/1003/1025#newcode15
Line 15: // The parent window for this bubble
period at end of sentence.

http://codereview.chromium.org/171016/diff/1003/1025#newcode18
Line 18: // The BBC need to know where to place itself; it asks the delegate
s/need/needs

http://codereview.chromium.org/171016/diff/1003/1025#newcode25
Line 25: // The bubble tells it's delegate when it's done and can be
deallocated.
s/tells it's/tells its/

http://codereview.chromium.org/171016/diff/1003/1025#newcode46
Line 46: NSWindow* window_; // strong
if it's strong, what about using scoped_nsobject<>? If you're relying on
"release-when-closed" comment that here. We don't really use that elsewhere, is
there any reason you're doing that here? Also, 2 spaces after the ; for
same-line comments.

http://codereview.chromium.org/171016/diff/1003/1025#newcode64
Line 64: - (id)initWithDelegate:(id<BookmarkBubbleControllerDelegate>)delegate
this needs a comment. what are |node| and |alreadybokmarked| used for? You
should mention here, rather than on -show, that this does not show the bubble
and that callers are responsible for calling -show.

http://codereview.chromium.org/171016/diff/1003/1025#newcode70
Line 70: // it desires it to be visible on the screen.  It is not shown by the
see above for where this comment best belongs.

http://codereview.chromium.org/171016/diff/1003/1025#newcode74
Line 74: // Actions for buttons in the dialog
period at end of sentence.

http://codereview.chromium.org/171016/diff/1003/1025#newcode81
Line 81: - (void)comboBoxSelectionDidChange:(NSNotification*)notification;
is this really a public method? Seems like an implementation detail.

http://codereview.chromium.org/171016/diff/1003/1025#newcode84
Line 84: - (void)windowDidResignKey:(NSNotification*)notification;
same as above.

http://codereview.chromium.org/171016/diff/1003/1025#newcode89
Line 89: @interface BookmarkBubbleController(PrivateAPI)
mention why it's private (for testing?)

http://codereview.chromium.org/171016/diff/1003/1025#newcode102
Line 102: 
remove extra blank line.

http://codereview.chromium.org/171016/diff/1003/1010
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/171016/diff/1003/1010#newcode63
Line 63: // NSButton only allows for a single key equivalent.  Fix.
can't you just implement "- (void)cancel:(id)sender" ? That's what we do in the
prefs window.

http://codereview.chromium.org/171016/diff/1003/1010#newcode70
Line 70: NSPoint origin = [delegate_ topLeftForBubble];
is this really something that changes over time? Can it be passed in the init?

http://codereview.chromium.org/171016/diff/1003/1010#newcode110
Line 110: NSString *parentTitle = base::SysWideToNSString(
chromium style is Foo* bar, not Foo *bar.

http://codereview.chromium.org/171016/diff/1003/1010#newcode119
Line 119: - (void)addFolderNodes:(const BookmarkNode*)node
toComboBox:(NSComboBox*)box {
parameter implies plural, but |node| is singular. Should it be renamed?

http://codereview.chromium.org/171016/diff/1003/1010#newcode121
Line 121: if ([title length])  { // no title if root
2 spaces after { for same-line comment

http://codereview.chromium.org/171016/diff/1003/1010#newcode125
Line 125: for (int i=0; i<node->GetChildCount(); i++) {
spaces around operators. |int i = 0; i < node->...|

http://codereview.chromium.org/171016/diff/1003/1010#newcode139
Line 139: // Testing API
maybe put all the testing api at the bottom so it's not interspersed with real
code?

http://codereview.chromium.org/171016/diff/1003/1005
File chrome/browser/cocoa/bookmark_bubble_view.mm (right):

http://codereview.chromium.org/171016/diff/1003/1005#newcode20
Line 20: // Make room for the border to be seen
period at end of sentence

http://codereview.chromium.org/171016/diff/1003/1005#newcode27
Line 27: NSBezierPath *bezier = [NSBezierPath bezierPath];
NSBezierPath* bezier =

http://codereview.chromium.org/171016/diff/1003/1017
File chrome/browser/cocoa/bookmark_bubble_window.mm (right):

http://codereview.chromium.org/171016/diff/1003/1017#newcode11
Line 11: // NSBorderlessWindowMask windows cannot become key or main.  In our
does this work correctly with Expose?

[John Grabowski, 2009-08-17 21:08:27]

>> If i remember this ui right, we'll need the layout tweaking support 
>> in here also to handle some localizations.

Likely; the bubble is pretty tight.

All comments addressed; new diffs up.

http://codereview.chromium.org/171016/diff/1003/1025
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/171016/diff/1003/1025#newcode15
Line 15: // The parent window for this bubble
On 2009/08/17 17:06:46, pink wrote:
> period at end of sentence.

.'ed

http://codereview.chromium.org/171016/diff/1003/1025#newcode18
Line 18: // The BBC need to know where to place itself; it asks the delegate
On 2009/08/17 17:06:46, pink wrote:
> s/need/needs

pluralified

http://codereview.chromium.org/171016/diff/1003/1025#newcode25
Line 25: // The bubble tells it's delegate when it's done and can be
deallocated.
On 2009/08/17 17:06:46, pink wrote:
> s/tells it's/tells its/

done

http://codereview.chromium.org/171016/diff/1003/1025#newcode46
Line 46: NSWindow* window_; // strong
On 2009/08/17 17:06:46, pink wrote:
> if it's strong, what about using scoped_nsobject<>? If you're relying on
> "release-when-closed" comment that here. We don't really use that elsewhere,
is
> there any reason you're doing that here? Also, 2 spaces after the ; for
> same-line comments.

Followed through (made scoped_nsobject, turned OFF release-when-closed, etc)

http://codereview.chromium.org/171016/diff/1003/1025#newcode64
Line 64: - (id)initWithDelegate:(id<BookmarkBubbleControllerDelegate>)delegate
On 2009/08/17 17:06:46, pink wrote:
> this needs a comment. what are |node| and |alreadybokmarked| used for? You
> should mention here, rather than on -show, that this does not show the bubble
> and that callers are responsible for calling -show.

Expanded

http://codereview.chromium.org/171016/diff/1003/1025#newcode70
Line 70: // it desires it to be visible on the screen.  It is not shown by the
On 2009/08/17 17:06:46, pink wrote:
> see above for where this comment best belongs.

Moved

http://codereview.chromium.org/171016/diff/1003/1025#newcode74
Line 74: // Actions for buttons in the dialog
On 2009/08/17 17:06:46, pink wrote:
> period at end of sentence.

.ed

http://codereview.chromium.org/171016/diff/1003/1025#newcode81
Line 81: - (void)comboBoxSelectionDidChange:(NSNotification*)notification;
On 2009/08/17 17:06:46, pink wrote:
> is this really a public method? Seems like an implementation detail.

It is called by the combo box, so visibility is technically public.
But I'll make it private.

http://codereview.chromium.org/171016/diff/1003/1025#newcode84
Line 84: - (void)windowDidResignKey:(NSNotification*)notification;
On 2009/08/17 17:06:46, pink wrote:
> same as above.

ditto

http://codereview.chromium.org/171016/diff/1003/1025#newcode89
Line 89: @interface BookmarkBubbleController(PrivateAPI)
On 2009/08/17 17:06:46, pink wrote:
> mention why it's private (for testing?)

Elaborated on (renamed)

http://codereview.chromium.org/171016/diff/1003/1025#newcode102
Line 102: 
On 2009/08/17 17:06:46, pink wrote:
> remove extra blank line.

removed

http://codereview.chromium.org/171016/diff/1003/1010
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/171016/diff/1003/1010#newcode5
Line 5: #include "app/l10n_util.h"
On 2009/08/17 01:23:18, TVL wrote:
> use l10n_util_mac?

done

http://codereview.chromium.org/171016/diff/1003/1010#newcode53
Line 53: l10n_util::GetString(IDS_BOOMARK_BUBBLE_PAGE_BOOKMARKED));
On 2009/08/17 01:23:18, TVL wrote:
> use l10n_util:GetNSString?

done

http://codereview.chromium.org/171016/diff/1003/1010#newcode63
Line 63: // NSButton only allows for a single key equivalent.  Fix.
On 2009/08/17 17:06:46, pink wrote:
> can't you just implement "- (void)cancel:(id)sender" ? That's what we do in
the
> prefs window.

Ah... yes.

http://codereview.chromium.org/171016/diff/1003/1010#newcode70
Line 70: NSPoint origin = [delegate_ topLeftForBubble];
On 2009/08/17 17:06:46, pink wrote:
> is this really something that changes over time? Can it be passed in the init?

Done (for both topLeft and the parent window)

http://codereview.chromium.org/171016/diff/1003/1010#newcode106
Line 106: retain]);
On 2009/08/17 01:23:18, TVL wrote:
> l10n_util::GetNSString?
> 
> Do you need worry about windows accelerator markers or ellipsis? 
> (l10n_util::FixUpWindowsStyleLabel)

Ellipsis.  Switches to l10n_util::GetNSString() to get it.

http://codereview.chromium.org/171016/diff/1003/1010#newcode110
Line 110: NSString *parentTitle = base::SysWideToNSString(
On 2009/08/17 17:06:46, pink wrote:
> chromium style is Foo* bar, not Foo *bar.

fixed

http://codereview.chromium.org/171016/diff/1003/1010#newcode119
Line 119: - (void)addFolderNodes:(const BookmarkNode*)node
toComboBox:(NSComboBox*)box {
On 2009/08/17 17:06:46, pink wrote:
> parameter implies plural, but |node| is singular. Should it be renamed?

Renamed to "parent".  Still singular I guess but is more clear that this is the
root.

http://codereview.chromium.org/171016/diff/1003/1010#newcode121
Line 121: if ([title length])  { // no title if root
On 2009/08/17 17:06:46, pink wrote:
> 2 spaces after { for same-line comment

added

http://codereview.chromium.org/171016/diff/1003/1010#newcode125
Line 125: for (int i=0; i<node->GetChildCount(); i++) {
On 2009/08/17 17:06:46, pink wrote:
> spaces around operators. |int i = 0; i < node->...|

spaced

http://codereview.chromium.org/171016/diff/1003/1010#newcode139
Line 139: // Testing API
On 2009/08/17 17:06:46, pink wrote:
> maybe put all the testing api at the bottom so it's not interspersed with real
> code?

Rearranged.  Also renamed TestingAPI to ExposedForUnitTesting since some of the
API is real but only exposed for...

http://codereview.chromium.org/171016/diff/1003/1010#newcode218
Line 218: // - If I @synthesize them in the category, it yells that it isn't
allowed.
On 2009/08/17 01:23:18, TVL wrote:
> can you do them in a @interface BookmarkBubbleController () decl?  objc2 added
> that for declaring methods have having the @impl checks for them (to support
> privates) so it might work on properties too.

It works; switched.

http://codereview.chromium.org/171016/diff/1003/1005
File chrome/browser/cocoa/bookmark_bubble_view.mm (right):

http://codereview.chromium.org/171016/diff/1003/1005#newcode20
Line 20: // Make room for the border to be seen
On 2009/08/17 17:06:46, pink wrote:
> period at end of sentence

Done (several spots in this file)

http://codereview.chromium.org/171016/diff/1003/1005#newcode27
Line 27: NSBezierPath *bezier = [NSBezierPath bezierPath];
On 2009/08/17 17:06:46, pink wrote:
> NSBezierPath* bezier = 

reformatted

http://codereview.chromium.org/171016/diff/1003/1017
File chrome/browser/cocoa/bookmark_bubble_window.mm (right):

http://codereview.chromium.org/171016/diff/1003/1017#newcode11
Line 11: // NSBorderlessWindowMask windows cannot become key or main.  In our
On 2009/08/17 17:06:46, pink wrote:
> does this work correctly with Expose?

Can you elaborate?  If I F11 then click somewhere then F11 again things are
fine.

[TVL, 2009-08-17 23:40:00]

On Mon, Aug 17, 2009 at 5:08 PM, <jrg@chromium.org> wrote:

> If i remember this ui right, we'll need the layout tweaking support
>>> in here also to handle some localizations.
>>>
>>
> Likely; the bubble is pretty tight.


Ok, want me to take a look once this lands?

TVL


>
>
> All comments addressed; new diffs up.
>
>
>
> http://codereview.chromium.org/171016/diff/1003/1025
> File chrome/browser/cocoa/bookmark_bubble_controller.h (right):
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode15
> Line 15: // The parent window for this bubble
> On 2009/08/17 17:06:46, pink wrote:
>
>> period at end of sentence.
>>
>
> .'ed
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode18
> Line 18: // The BBC need to know where to place itself; it asks the
> delegate
> On 2009/08/17 17:06:46, pink wrote:
>
>> s/need/needs
>>
>
> pluralified
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode25
> Line 25: // The bubble tells it's delegate when it's done and can be
> deallocated.
> On 2009/08/17 17:06:46, pink wrote:
>
>> s/tells it's/tells its/
>>
>
> done
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode46
> Line 46: NSWindow* window_; // strong
> On 2009/08/17 17:06:46, pink wrote:
>
>> if it's strong, what about using scoped_nsobject<>? If you're relying
>>
> on
>
>> "release-when-closed" comment that here. We don't really use that
>>
> elsewhere, is
>
>> there any reason you're doing that here? Also, 2 spaces after the ;
>>
> for
>
>> same-line comments.
>>
>
> Followed through (made scoped_nsobject, turned OFF release-when-closed,
> etc)
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode64
> Line 64: -
> (id)initWithDelegate:(id<BookmarkBubbleControllerDelegate>)delegate
> On 2009/08/17 17:06:46, pink wrote:
>
>> this needs a comment. what are |node| and |alreadybokmarked| used for?
>>
> You
>
>> should mention here, rather than on -show, that this does not show the
>>
> bubble
>
>> and that callers are responsible for calling -show.
>>
>
> Expanded
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode70
> Line 70: // it desires it to be visible on the screen.  It is not shown
> by the
> On 2009/08/17 17:06:46, pink wrote:
>
>> see above for where this comment best belongs.
>>
>
> Moved
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode74
> Line 74: // Actions for buttons in the dialog
> On 2009/08/17 17:06:46, pink wrote:
>
>> period at end of sentence.
>>
>
> .ed
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode81
> Line 81: -
> (void)comboBoxSelectionDidChange:(NSNotification*)notification;
> On 2009/08/17 17:06:46, pink wrote:
>
>> is this really a public method? Seems like an implementation detail.
>>
>
> It is called by the combo box, so visibility is technically public.
> But I'll make it private.
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode84
> Line 84: - (void)windowDidResignKey:(NSNotification*)notification;
> On 2009/08/17 17:06:46, pink wrote:
>
>> same as above.
>>
>
> ditto
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode89
> Line 89: @interface BookmarkBubbleController(PrivateAPI)
> On 2009/08/17 17:06:46, pink wrote:
>
>> mention why it's private (for testing?)
>>
>
> Elaborated on (renamed)
>
> http://codereview.chromium.org/171016/diff/1003/1025#newcode102
> Line 102:
> On 2009/08/17 17:06:46, pink wrote:
>
>> remove extra blank line.
>>
>
> removed
>
> http://codereview.chromium.org/171016/diff/1003/1010
> File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode5
> Line 5: #include "app/l10n_util.h"
> On 2009/08/17 01:23:18, TVL wrote:
>
>> use l10n_util_mac?
>>
>
> done
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode53
> Line 53: l10n_util::GetString(IDS_BOOMARK_BUBBLE_PAGE_BOOKMARKED));
> On 2009/08/17 01:23:18, TVL wrote:
>
>> use l10n_util:GetNSString?
>>
>
> done
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode63
> Line 63: // NSButton only allows for a single key equivalent.  Fix.
> On 2009/08/17 17:06:46, pink wrote:
>
>> can't you just implement "- (void)cancel:(id)sender" ? That's what we
>>
> do in the
>
>> prefs window.
>>
>
> Ah... yes.
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode70
> Line 70: NSPoint origin = [delegate_ topLeftForBubble];
> On 2009/08/17 17:06:46, pink wrote:
>
>> is this really something that changes over time? Can it be passed in
>>
> the init?
>
> Done (for both topLeft and the parent window)
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode106
> Line 106: retain]);
> On 2009/08/17 01:23:18, TVL wrote:
>
>> l10n_util::GetNSString?
>>
>
>  Do you need worry about windows accelerator markers or ellipsis?
>> (l10n_util::FixUpWindowsStyleLabel)
>>
>
> Ellipsis.  Switches to l10n_util::GetNSString() to get it.
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode110
> Line 110: NSString *parentTitle = base::SysWideToNSString(
> On 2009/08/17 17:06:46, pink wrote:
>
>> chromium style is Foo* bar, not Foo *bar.
>>
>
> fixed
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode119
> Line 119: - (void)addFolderNodes:(const BookmarkNode*)node
> toComboBox:(NSComboBox*)box {
> On 2009/08/17 17:06:46, pink wrote:
>
>> parameter implies plural, but |node| is singular. Should it be
>>
> renamed?
>
> Renamed to "parent".  Still singular I guess but is more clear that this
> is the root.
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode121
> Line 121: if ([title length])  { // no title if root
> On 2009/08/17 17:06:46, pink wrote:
>
>> 2 spaces after { for same-line comment
>>
>
> added
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode125
> Line 125: for (int i=0; i<node->GetChildCount(); i++) {
> On 2009/08/17 17:06:46, pink wrote:
>
>> spaces around operators. |int i = 0; i < node->...|
>>
>
> spaced
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode139
> Line 139: // Testing API
> On 2009/08/17 17:06:46, pink wrote:
>
>> maybe put all the testing api at the bottom so it's not interspersed
>>
> with real
>
>> code?
>>
>
> Rearranged.  Also renamed TestingAPI to ExposedForUnitTesting since some
> of the API is real but only exposed for...
>
> http://codereview.chromium.org/171016/diff/1003/1010#newcode218
> Line 218: // - If I @synthesize them in the category, it yells that it
> isn't allowed.
> On 2009/08/17 01:23:18, TVL wrote:
>
>> can you do them in a @interface BookmarkBubbleController () decl?
>>
> objc2 added
>
>> that for declaring methods have having the @impl checks for them (to
>>
> support
>
>> privates) so it might work on properties too.
>>
>
> It works; switched.
>
> http://codereview.chromium.org/171016/diff/1003/1005
> File chrome/browser/cocoa/bookmark_bubble_view.mm (right):
>
> http://codereview.chromium.org/171016/diff/1003/1005#newcode20
> Line 20: // Make room for the border to be seen
> On 2009/08/17 17:06:46, pink wrote:
>
>> period at end of sentence
>>
>
> Done (several spots in this file)
>
> http://codereview.chromium.org/171016/diff/1003/1005#newcode27
> Line 27: NSBezierPath *bezier = [NSBezierPath bezierPath];
> On 2009/08/17 17:06:46, pink wrote:
>
>> NSBezierPath* bezier =
>>
>
> reformatted
>
> http://codereview.chromium.org/171016/diff/1003/1017
> File chrome/browser/cocoa/bookmark_bubble_window.mm (right):
>
> http://codereview.chromium.org/171016/diff/1003/1017#newcode11
> Line 11: // NSBorderlessWindowMask windows cannot become key or main.
> In our
> On 2009/08/17 17:06:46, pink wrote:
>
>> does this work correctly with Expose?
>>
>
> Can you elaborate?  If I F11 then click somewhere then F11 again things
> are fine.
>
>
> http://codereview.chromium.org/171016
>

[pink (ping after 24hrs), 2009-08-19 15:46:02]

LGTM with these corrected.

http://codereview.chromium.org/171016/diff/1030/1052
File chrome/browser/cocoa/bookmark_bubble_controller.h (right):

http://codereview.chromium.org/171016/diff/1030/1052#newcode81
Line 81: - (IBAction)cancel:(id)sender;
still not technically required to be in the public API as it'll be called
regardless.

http://codereview.chromium.org/171016/diff/1030/1037
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/171016/diff/1030/1037#newcode39
Line 39: titleMapping_.reset([[NSMutableDictionary alloc] initWithCapacity:1]);
why bother using initWithCapacity if you're only going to init with 1 item?

http://codereview.chromium.org/171016/diff/1030/1037#newcode55
Line 55: - (void)dealloc {
i'd move dealloc and awakeFromNib right below init so they're together.

http://codereview.chromium.org/171016/diff/1030/1037#newcode132
Line 132: contentRect = NSOffsetRect(contentRect,
what about using NSWindow convertBaseToScreen: here?

http://codereview.chromium.org/171016/diff/1030/1037#newcode168
Line 168: // For the given folder node, walk the tree and add folder names to
so this just flattens any hierarchy? wouldn't that make it harder to use? What
do we do on windows?

http://codereview.chromium.org/171016/diff/1030/1051
File chrome/browser/cocoa/bookmark_bubble_controller_unittest.mm (right):

http://codereview.chromium.org/171016/diff/1030/1051#newcode51
Line 51: class BookmarkBubbleControllerTest : public testing::Test {
should this inherit from PlatformTest to get an autorelease pool? Does anything
need it?

http://codereview.chromium.org/171016/diff/1030/1040
File chrome/browser/cocoa/bookmark_bubble_view_unittest.mm (right):

http://codereview.chromium.org/171016/diff/1030/1040#newcode14
Line 14: class BookmarkBubbleViewTest : public testing::Test {
PlatformTest for autorelease pool? I think the drawing code uses autoreleased
objects for sure.

http://codereview.chromium.org/171016/diff/1030/1039
File chrome/browser/cocoa/browser_window_controller.mm (right):

http://codereview.chromium.org/171016/diff/1030/1039#newcode946
Line 946: NSPoint p = NSMakePoint(NSMinX(rect), NSMinY(rect)); // bottom left
2 spaces after semicolon

http://codereview.chromium.org/171016/diff/1030/1039#newcode950
Line 950: // Show the bookmark bubble (e.g. user just clicked on the STAR)
period at end of sentence.

http://codereview.chromium.org/171016/diff/1030/1039#newcode988
Line 988: [bookmarkBubbleController_ closeWindow];
dealloc already does this so you shouldn't need to here. Do you want to document
that in the controller's header?

[John Grabowski, 2009-08-19 22:47:39]

All feedback applied; only one item has an interesting reply.
Thx pink.

http://codereview.chromium.org/171016/diff/1030/1037
File chrome/browser/cocoa/bookmark_bubble_controller.mm (right):

http://codereview.chromium.org/171016/diff/1030/1037#newcode168
Line 168: // For the given folder node, walk the tree and add folder names to
On 2009/08/19 15:46:02, pink wrote:
> so this just flattens any hierarchy? wouldn't that make it harder to use? What
> do we do on windows?

Same on Windows.  (However, on windows, duplicated named do NOT point to the
same folder, which isn't something we do properly on the Mac).  I will fix with
the TODO.