Change the folder presentation in the Bookmark Editor and the Bookmark All Ta...

Change the folder presentation in the Bookmark Editor and the Bookmark All Tabs dialogs to a tree view.

Nib changes: Removed the NSBrowser and added an NSOutlineView.

BUG=26647, 26643, 26718, 27634
TEST=Bring up the bookmark editor by control-clicking in the bookmarks bar and selecting Add Page... or by selecting the Bookmark this Page... menu item found in the Bookmarks menu.  Observe that the folder presentation is now a tree view.  Select any folder and click New Folder to verify that new folders can be added.  Double-click on the newly created folder to change its name.  Added folders will not commit until OK is pressed, which will require that a bookmark actually be added.  Also bring up the Bookmark All Tabs dialog by control-clicking in the tab bar with more than one tab open and verify that the folder structure is shown in a tree view.



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

[mrossetti, 2009-11-13 06:53:46]

Changing to an NSOutlineView dramatically simplifies the implementation.

[TVL, 2009-11-13 12:49:27]

changed started out with a change to base/mac_util.mm?

[TVL, 2009-11-13 13:08:58]

http://codereview.chromium.org/393006/diff/3/1005
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/3/1005#newcode98
Line 98: #if 0
remove?

http://codereview.chromium.org/393006/diff/3/1007
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/3/1007#newcode172
Line 172: - (id)selectedFolder {
why id?

http://codereview.chromium.org/393006/diff/3/1007#newcode237
Line 237: NSIndexPath*path = nil;
space after *

http://codereview.chromium.org/393006/diff/3/1007#newcode245
Line 245: const BookmarkNode* node = static_cast<const
BookmarkNode*>([nodePointer pointerValue]);
80col

http://codereview.chromium.org/393006/diff/3/1007#newcode253
Line 253: path:testPath];
quick glance, this looks like something n^2? since it seems to crawl all over
the tree trying to find it.  can't we get the parents out of desiredNode to walk
directly?

http://codereview.chromium.org/393006/diff/3/1009
File chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/3/1009#newcode15
Line 15: class BookmarkEditorBaseControllerTest : public PlatformTest {
why these changes?

[John Grabowski, 2009-11-13 17:39:34]

http://codereview.chromium.org/393006/diff/3/1004
File chrome/browser/cocoa/bookmark_all_tabs_controller.mm (right):

http://codereview.chromium.org/393006/diff/3/1004#newcode59
Line 59: [super ok:sender];
Are you sure you want this first and not last?  It'll call [NSApplication
endSheet:].  Normally, if a window (and view) goes away, it's not a good idea to
look at UI elements on it (e.g. [self selectedNode]).

http://codereview.chromium.org/393006/diff/3/1005
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/3/1005#newcode40
Line 40: scoped_nsobject<NSArray> folderTreeArray_;
A little doc on these?  E.g. "array of dictionaries with specific keys blah
blah..."

http://codereview.chromium.org/393006/diff/3/1005#newcode47
Line 47: @property (copy) NSArray* folderTreeArray;
If you want copy, perhaps you also want deep copy?  This is array of
dictionaries.

http://codereview.chromium.org/393006/diff/3/1007
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/3/1007#newcode151
Line 151: - (id)folderForIndexPath:(NSIndexPath*)indexPath {
why return id and not NSDictionary?

http://codereview.chromium.org/393006/diff/3/1007#newcode358
Line 358: // TODO(mrossetti): Figure out how to put the 'New folder' cell into
edit.
What does this TODO mean?  Does it mean you can't edit the name of the new
folder?

http://codereview.chromium.org/393006/diff/3/1007#newcode367
Line 367: // Scan the tree looking for nodes marked 'newFolder' and create those
nodes.
Looks like you're making an assumption about state.  Looks like lots of state is
maintained here.  Would be good to document such state in the header so things
are easier to follow from the big picture perspective.

http://codereview.chromium.org/393006/diff/3/1009
File chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/3/1009#newcode15
Line 15: class BookmarkEditorBaseControllerTest : public PlatformTest {
On 2009/11/13 13:08:58, TVL wrote:
> why these changes?

I agree; I think we're in general switching from PlatformTest to CocoaTest.

http://codereview.chromium.org/393006/diff/3/1009#newcode132
Line 132: }
Dude, you added a million lines of new code but only 6 lines of test?  What did
you expect my response to be?

http://codereview.chromium.org/393006/diff/3/1008
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/393006/diff/3/1008#newcode92
Line 92: [super ok:sender];
I think you should call at the end...

http://codereview.chromium.org/393006/diff/3/1006
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/3/1006#newcode15
Line 15: class BookmarkEditorControllerTest : public PlatformTest {
See other comment about transition TO CocoaTest.

http://codereview.chromium.org/393006/diff/3/1006#newcode17
Line 17: CocoaTestHelper cocoa_helper_;  // Inits Cocoa, creates window, etc...
See cocoa_test_helper.h for deprecation warning about CocoaTestHeper

[mrossetti, 2009-11-13 23:57:35]

Issues addressed!

I'm going to be out until late this evening so I won't be around to remedy any
issues you find with them until then. I'd be very grateful if there are only
cosmetic or simple issues to fix if you would consider LGing them with the
assurance I'll fix them.

http://codereview.chromium.org/393006/diff/3/1004
File chrome/browser/cocoa/bookmark_all_tabs_controller.mm (right):

http://codereview.chromium.org/393006/diff/3/1004#newcode59
Line 59: [super ok:sender];
On 2009/11/13 17:39:34, John Grabowski wrote:
> Are you sure you want this first and not last?  It'll call [NSApplication
> endSheet:].  Normally, if a window (and view) goes away, it's not a good idea
to
> look at UI elements on it (e.g. [self selectedNode]).

Fixed by abstracting out the folder creation code into a new function.

http://codereview.chromium.org/393006/diff/3/1005
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/3/1005#newcode40
Line 40: scoped_nsobject<NSArray> folderTreeArray_;
On 2009/11/13 17:39:34, John Grabowski wrote:
> A little doc on these?  E.g. "array of dictionaries with specific keys blah
> blah..."

Done.

http://codereview.chromium.org/393006/diff/3/1005#newcode47
Line 47: @property (copy) NSArray* folderTreeArray;
On 2009/11/13 17:39:34, John Grabowski wrote:
> If you want copy, perhaps you also want deep copy?  This is array of
> dictionaries.

Fixed properties, removed setFolderTreeArray, and directly assigned the only
instance.

http://codereview.chromium.org/393006/diff/3/1005#newcode98
Line 98: #if 0
On 2009/11/13 13:08:58, TVL wrote:
> remove?

Gah.  Removed.

http://codereview.chromium.org/393006/diff/3/1007
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/3/1007#newcode151
Line 151: - (id)folderForIndexPath:(NSIndexPath*)indexPath {
On 2009/11/13 17:39:34, John Grabowski wrote:
> why return id and not NSDictionary?

Done.  Originally, this was going to be more generic but it ended up that it
couldn't be done more generically without getting bogus.

http://codereview.chromium.org/393006/diff/3/1007#newcode172
Line 172: - (id)selectedFolder {
On 2009/11/13 13:08:58, TVL wrote:
> why id?

Changed to NSMutableDictionary*.  This was originally generic code but I later
realized that it relied on the structure of the dictionary.

http://codereview.chromium.org/393006/diff/3/1007#newcode237
Line 237: NSIndexPath*path = nil;
On 2009/11/13 13:08:58, TVL wrote:
> space after *

Done.

http://codereview.chromium.org/393006/diff/3/1007#newcode245
Line 245: const BookmarkNode* node = static_cast<const
BookmarkNode*>([nodePointer pointerValue]);
On 2009/11/13 13:08:58, TVL wrote:
> 80col

Done.

http://codereview.chromium.org/393006/diff/3/1007#newcode253
Line 253: path:testPath];
On 2009/11/13 13:08:58, TVL wrote:
> quick glance, this looks like something n^2? since it seems to crawl all over
> the tree trying to find it.  can't we get the parents out of desiredNode to
walk
> directly?

Done.

http://codereview.chromium.org/393006/diff/3/1007#newcode358
Line 358: // TODO(mrossetti): Figure out how to put the 'New folder' cell into
edit.
On 2009/11/13 17:39:34, John Grabowski wrote:
> What does this TODO mean?  Does it mean you can't edit the name of the new
> folder?

Fixed.

http://codereview.chromium.org/393006/diff/3/1007#newcode367
Line 367: // Scan the tree looking for nodes marked 'newFolder' and create those
nodes.
On 2009/11/13 17:39:34, John Grabowski wrote:
> Looks like you're making an assumption about state.  Looks like lots of state
is
> maintained here.  Would be good to document such state in the header so things
> are easier to follow from the big picture perspective.

Done.

http://codereview.chromium.org/393006/diff/3/1009
File chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/3/1009#newcode15
Line 15: class BookmarkEditorBaseControllerTest : public PlatformTest {
On 2009/11/13 13:08:58, TVL wrote:
> why these changes?

t this point (meaning in order to get this out the door for M4), I'm using
PlatformTest due to consistent crashes in PlatformTest's cleanup when routed
through CocoaTest.  I've added a bug to track this post-M4.

http://codereview.chromium.org/393006/diff/3/1009#newcode15
Line 15: class BookmarkEditorBaseControllerTest : public PlatformTest {
On 2009/11/13 17:39:34, John Grabowski wrote:
> On 2009/11/13 13:08:58, TVL wrote:
> > why these changes?
> 
> I agree; I think we're in general switching from PlatformTest to CocoaTest.

See previous comment.

http://codereview.chromium.org/393006/diff/3/1009#newcode132
Line 132: }
On 2009/11/13 17:39:34, John Grabowski wrote:
> Dude, you added a million lines of new code but only 6 lines of test?  What
did
> you expect my response to be?

Done.

http://codereview.chromium.org/393006/diff/3/1008
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/393006/diff/3/1008#newcode92
Line 92: [super ok:sender];
On 2009/11/13 17:39:34, John Grabowski wrote:
> I think you should call at the end...

Refactored and fixed.

http://codereview.chromium.org/393006/diff/3/1006
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/3/1006#newcode15
Line 15: class BookmarkEditorControllerTest : public PlatformTest {
On 2009/11/13 17:39:34, John Grabowski wrote:
> See other comment about transition TO CocoaTest.
> 

Added TODO referring to http://crbug.com/27634.

http://codereview.chromium.org/393006/diff/3/1006#newcode17
Line 17: CocoaTestHelper cocoa_helper_;  // Inits Cocoa, creates window, etc...
On 2009/11/13 17:39:34, John Grabowski wrote:
> See cocoa_test_helper.h for deprecation warning about CocoaTestHeper

Added TODO referring to http://crbug.com/27634.

[John Grabowski, 2009-11-15 21:05:24]

Please fix the CocoaTest thing and I'll LGTM you.

I don't like the use of dict instead of a new class but that probably isn't more
than stylistic preference.

http://codereview.chromium.org/393006/diff/4003/8009
File chrome/browser/cocoa/bookmark_all_tabs_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/4003/8009#newcode34
Line 34: // TODO(mrossetti): Use CocoaTest http://crbug.com/27634
To be honest I don't like this.  It used to be CocoaTest and was made
PlatformTest.  tvl and I said "don't do that please", and you added a TODO.
Is it really that hard to switch back?  I'd think you could revert all changes
except
  [controller selectTestNodeInBrowser:group_a_];
and it would be done.  Yes?

http://codereview.chromium.org/393006/diff/4003/8004
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/4003/8004#newcode40
Line 40: // An array of dictionaries where each dictionary describes a folder in
the
(Not sure if I'm repeating a comment from a different CL, or I somehow forgot to
add it to initial comments, but...)
Instead of having an array of dicts with 4 keys, why not an array of @class
objects that have 4 members?  Isn't that what classes are for?  Would make
reading of the code more clear.
Yes, we could have a dict with keys to substitute for almost any struct-type
object, but I'm not sure it's a good idea.  The win of a dict with keys is for
situations where the members aren't known in advance (e.g. preferences) or where
it'll change (e.g. library API).  I don't think either of those applies here.

http://codereview.chromium.org/393006/diff/4003/8004#newcode72
Line 72: // The cencel action will dismiss the dialog.  Your derived class, if
it
s/cencel/cancel/g

http://codereview.chromium.org/393006/diff/4003/8004#newcode75
Line 75: - (IBAction)cancel:(id)sender;
Would be nice to do this chaining automatically so derived classes can't forget.
 Remember PowerPlant's Blah() and BlahSelf()?  E.g. bind IB to _privateOk: which
calls a derived ok:, then _privateSelfOk:.

http://codereview.chromium.org/393006/diff/4003/8008
File chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/4003/8008#newcode18
Line 18: class BookmarkEditorBaseControllerTest : public PlatformTest {
See other comment in this CL about CocoaTest use

http://codereview.chromium.org/393006/diff/4003/8005
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/4003/8005#newcode16
Line 16: class BookmarkEditorControllerTest : public PlatformTest {
Ditto CocoaTest comment

[pink (ping after 24hrs), 2009-11-17 15:08:57]

drive-by

http://codereview.chromium.org/393006/diff/4003/8004
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/4003/8004#newcode40
Line 40: // An array of dictionaries where each dictionary describes a folder in
the
On 2009/11/15 21:05:24, John Grabowski wrote:
> (Not sure if I'm repeating a comment from a different CL, or I somehow forgot
to
> add it to initial comments, but...)
> Instead of having an array of dicts with 4 keys, why not an array of @class
> objects that have 4 members?  Isn't that what classes are for? 

I agree, we shouldn't be using dictionaries for this. There's nothing that
guarantees that the dictionary entries will be configured correctly or that the
keys make an object that is internally consistent. An object with init/dealloc
can ensure this and can be unit tested.

http://codereview.chromium.org/393006/diff/4003/8004#newcode77
Line 77: // The OK action will dismiss the dialog.  Your derived class, if it
I'd avoid the use of 2nd person here. You can say "Any derived class..." and get
the same meaning.

http://codereview.chromium.org/393006/diff/4003/8004#newcode89
Line 89: // dictionary item is YES).  Your derived class must call this
Same comment as above about 2nd person.

http://codereview.chromium.org/393006/diff/4003/8006
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/4003/8006#newcode313
Line 313: for (NSMutableDictionary *subFolderDict in subfolders) {
NSMutableDictionary*

http://codereview.chromium.org/393006/diff/4003/8006#newcode398
Line 398: for (NSMutableDictionary *folderDict in folderTreeArray) {
NSMutableDictionary*

[mrossetti, 2009-11-17 23:22:09]

This has been a lot of fun.  Thanks for all the great observations!

http://codereview.chromium.org/393006/diff/4003/8009
File chrome/browser/cocoa/bookmark_all_tabs_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/4003/8009#newcode34
Line 34: // TODO(mrossetti): Use CocoaTest http://crbug.com/27634
On 2009/11/15 21:05:24, John Grabowski wrote:
> To be honest I don't like this.  It used to be CocoaTest and was made
> PlatformTest.  tvl and I said "don't do that please", and you added a TODO.
> Is it really that hard to switch back?  I'd think you could revert all changes
> except
>   [controller selectTestNodeInBrowser:group_a_];
> and it would be done.  Yes?

Done.

http://codereview.chromium.org/393006/diff/4003/8004
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/4003/8004#newcode40
Line 40: // An array of dictionaries where each dictionary describes a folder in
the
On 2009/11/15 21:05:24, John Grabowski wrote:
> (Not sure if I'm repeating a comment from a different CL, or I somehow forgot
to
> add it to initial comments, but...)
> Instead of having an array of dicts with 4 keys, why not an array of @class
> objects that have 4 members?  Isn't that what classes are for?  Would make
> reading of the code more clear.
> Yes, we could have a dict with keys to substitute for almost any struct-type
> object, but I'm not sure it's a good idea.  The win of a dict with keys is for
> situations where the members aren't known in advance (e.g. preferences) or
where
> it'll change (e.g. library API).  I don't think either of those applies here.

Done.

http://codereview.chromium.org/393006/diff/4003/8004#newcode40
Line 40: // An array of dictionaries where each dictionary describes a folder in
the
On 2009/11/17 15:08:57, pink wrote:
> On 2009/11/15 21:05:24, John Grabowski wrote:
> > (Not sure if I'm repeating a comment from a different CL, or I somehow
forgot
> to
> > add it to initial comments, but...)
> > Instead of having an array of dicts with 4 keys, why not an array of @class
> > objects that have 4 members?  Isn't that what classes are for? 
> 
> I agree, we shouldn't be using dictionaries for this. There's nothing that
> guarantees that the dictionary entries will be configured correctly or that
the
> keys make an object that is internally consistent. An object with init/dealloc
> can ensure this and can be unit tested.
> 

What?  You don't want to take full advantage of the dynamic nature of Obj-C? 
;^)

Already changed and will be in the next review.

http://codereview.chromium.org/393006/diff/4003/8004#newcode72
Line 72: // The cencel action will dismiss the dialog.  Your derived class, if
it
On 2009/11/15 21:05:24, John Grabowski wrote:
> s/cencel/cancel/g
> 

Done.

http://codereview.chromium.org/393006/diff/4003/8004#newcode75
Line 75: - (IBAction)cancel:(id)sender;
On 2009/11/15 21:05:24, John Grabowski wrote:
> Would be nice to do this chaining automatically so derived classes can't
forget.
>  Remember PowerPlant's Blah() and BlahSelf()?  E.g. bind IB to _privateOk:
which
> calls a derived ok:, then _privateSelfOk:.  

Done.

http://codereview.chromium.org/393006/diff/4003/8004#newcode77
Line 77: // The OK action will dismiss the dialog.  Your derived class, if it
On 2009/11/17 15:08:57, pink wrote:
> I'd avoid the use of 2nd person here. You can say "Any derived class..." and
get
> the same meaning.

Done.

http://codereview.chromium.org/393006/diff/4003/8004#newcode89
Line 89: // dictionary item is YES).  Your derived class must call this
On 2009/11/17 15:08:57, pink wrote:
> Same comment as above about 2nd person.

Done.

http://codereview.chromium.org/393006/diff/4003/8006
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/4003/8006#newcode313
Line 313: for (NSMutableDictionary *subFolderDict in subfolders) {
On 2009/11/17 15:08:57, pink wrote:
> NSMutableDictionary*

Done.

http://codereview.chromium.org/393006/diff/4003/8006#newcode398
Line 398: for (NSMutableDictionary *folderDict in folderTreeArray) {
On 2009/11/17 15:08:57, pink wrote:
> NSMutableDictionary*

Done.

http://codereview.chromium.org/393006/diff/4003/8008
File chrome/browser/cocoa/bookmark_editor_base_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/4003/8008#newcode18
Line 18: class BookmarkEditorBaseControllerTest : public PlatformTest {
On 2009/11/15 21:05:24, John Grabowski wrote:
> See other comment in this CL about CocoaTest use
> 

Done.

http://codereview.chromium.org/393006/diff/4003/8005
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/393006/diff/4003/8005#newcode16
Line 16: class BookmarkEditorControllerTest : public PlatformTest {
On 2009/11/15 21:05:24, John Grabowski wrote:
> Ditto CocoaTest comment

Done.

[John Grabowski, 2009-11-18 18:48:21]

LGTM if memory problems in bookmark_editor_base_controller.mm fixed.

Re: NSNumber and not BOOL.  Sadness but I understand.

http://codereview.chromium.org/393006/diff/10002/12003
File chrome/browser/cocoa/bookmark_all_tabs_controller.mm (right):

http://codereview.chromium.org/393006/diff/10002/12003#newcode55
Line 55: // folder for the tabs and then the bookmarks in that new folder.
And returns....

http://codereview.chromium.org/393006/diff/10002/12005
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/10002/12005#newcode82
Line 82: // indicating if the operation should be allowed to continue.
Why NSNumber and not just BOOL?

http://codereview.chromium.org/393006/diff/10002/12005#newcode115
Line 115: NSMutableArray* children_;
Looks strong, yes?  Would like to use scoped_nsobject... but then can't
@property I guess.

http://codereview.chromium.org/393006/diff/10002/12006
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/10002/12006#newcode437
Line 437: folderName_ = [folderName copy];
If the gloves don't fit you must acquit.
If you copy then please you must release.

http://codereview.chromium.org/393006/diff/10002/12006#newcode439
Line 439: children_ = [children retain];
Need dealloc to have [children_ release], yes?

[mrossetti, 2009-11-18 19:25:33]

http://codereview.chromium.org/393006/diff/10002/12003
File chrome/browser/cocoa/bookmark_all_tabs_controller.mm (right):

http://codereview.chromium.org/393006/diff/10002/12003#newcode55
Line 55: // folder for the tabs and then the bookmarks in that new folder.
On 2009/11/18 18:48:21, John Grabowski wrote:
> And returns....

Done.

http://codereview.chromium.org/393006/diff/10002/12005
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/10002/12005#newcode82
Line 82: // indicating if the operation should be allowed to continue.
According to the docs on performSelector: the return value must be an object. 
The signature is -(id)performSelector:...

On 2009/11/18 18:48:21, John Grabowski wrote:
> Why NSNumber and not just BOOL?

http://codereview.chromium.org/393006/diff/10002/12005#newcode115
Line 115: NSMutableArray* children_;
It's a conundrum.  Added the missing dealloc.  <sigh>

On 2009/11/18 18:48:21, John Grabowski wrote:
> Looks strong, yes?  Would like to use scoped_nsobject... but then can't
> @property I guess.

http://codereview.chromium.org/393006/diff/10002/12006
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/10002/12006#newcode437
Line 437: folderName_ = [folderName copy];
On 2009/11/18 18:48:21, John Grabowski wrote:
> If the gloves don't fit you must acquit.
> If you copy then please you must release.

Added missing dealloc.

http://codereview.chromium.org/393006/diff/10002/12006#newcode439
Line 439: children_ = [children retain];
On 2009/11/18 18:48:21, John Grabowski wrote:
> Need dealloc to have [children_ release], yes?

Added missing dealloc.

[mrossetti, 2009-11-18 22:00:04]

Please take one  more quick look at the changes in
bookmark_editor_base_controller.h/.mm for BookmarkFolderInfo's init...
functions.

[pink (ping after 24hrs), 2009-11-18 23:11:49]

lgtm

http://codereview.chromium.org/393006/diff/12013/12018
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/393006/diff/12013/12018#newcode115
Line 115: const BookmarkNode* folderNode_;
weak reference

http://codereview.chromium.org/393006/diff/12013/12019
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/393006/diff/12013/12019#newcode41
Line 41: // static; implemented for each platform.  Update this function for new
"update this function for new new classes"?

[John Grabowski, 2009-11-18 23:22:17]

Still LGTM

On Wed, Nov 18, 2009 at 2:00 PM, <mrossetti@chromium.org> wrote:

> Please take one  more quick look at the changes in
> bookmark_editor_base_controller.h/.mm for BookmarkFolderInfo's init...
> functions.
>
>
> http://codereview.chromium.org/393006
>