Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
501073:
Native Cocoa bookmark manager, part 1 (Closed)
|
Created:
11 years ago by Jens Alfke Modified:
9 years, 7 months ago CC:
chromium-reviews_googlegroups.com, John Grabowski, pam+watch_chromium.org, ben+cc_chromium.org, Paweł Hajdan Jr. Visibility:
Public. |
DescriptionNative Cocoa bookmark manager, part 1
Most features are implemented, but Recents, Search and Undo are still missing.
MainMenu.xib: Just added "Bookmark Manager" menu item at top of "Bookmarks" menu.
BookmarkManager.xib: New file, containing Bookmark Manager panel, owned by BookmarkManagerController class.
BUG=13149
TEST=New unit tests for each new class.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=35546
Patch Set 1 #
Total comments: 121
Patch Set 2 : Lots of coding-style tweaks, and fixed some bugs noted in earlier comments. #
Total comments: 93
Patch Set 3 : Responding to latest comments. #Patch Set 4 : Preliminary unit tests. Minor bug fixes. #
Total comments: 13
Patch Set 5 : Style fixes, and copy/paste unit tests #
Total comments: 3
Messages
Total messages: 23 (0 generated)
Here's the native bookmark manager. It's running well aside from missing a few features; I can implement those in a few more days.
Looking -- thanks. Quick comments: 1. You should also add stuff in app_controller_mac.mm to enable the menu item when no window is open. 2. Due to the pain of looking at xib file diffs), please include a brief description of modifications of (existing) xib files in the description.
Jens, could you go through and correct all the stylistic nits? That way we can have a review without the important stuff buried amid trivial things. Thanks. http://codereview.chromium.org/501073/diff/1/6 File chrome/browser/cocoa/bookmark_groups_controller.h (right): http://codereview.chromium.org/501073/diff/1/6#newcode6 chrome/browser/cocoa/bookmark_groups_controller.h:6: @class BookmarkManager; Blank line between the #import and the @class, please. http://codereview.chromium.org/501073/diff/1/6#newcode9 chrome/browser/cocoa/bookmark_groups_controller.h:9: Unless the file is big and hairy, we can probably do without the double linebreaks. http://codereview.chromium.org/501073/diff/1/6#newcode11 chrome/browser/cocoa/bookmark_groups_controller.h:11: // Controller for the bookmark group list (the left pane). The left pane of what? http://codereview.chromium.org/501073/diff/1/6#newcode14 chrome/browser/cocoa/bookmark_groups_controller.h:14: IBOutlet NSTableView *groupsTable_; Probably |NSTableView* groupsTable_| (* attached to the type). We can accept either, but consistency within each file is key (and in our code we nearly universally attach the * to the type). http://codereview.chromium.org/501073/diff/1/6#newcode24 chrome/browser/cocoa/bookmark_groups_controller.h:24: - (IBAction)tableClicked:(id)sender; Each public method should have a descriptive comment. http://codereview.chromium.org/501073/diff/1/6#newcode26 chrome/browser/cocoa/bookmark_groups_controller.h:26: - (void) reload; |(void)reload| (no space). http://codereview.chromium.org/501073/diff/1/6#newcode31 chrome/browser/cocoa/bookmark_groups_controller.h:31: Here too. http://codereview.chromium.org/501073/diff/1/7 File chrome/browser/cocoa/bookmark_groups_controller.mm (right): http://codereview.chromium.org/501073/diff/1/7#newcode5 chrome/browser/cocoa/bookmark_groups_controller.mm:5: #import "chrome/browser/cocoa/bookmark_groups_controller.h" Probably a newline after this (to separate it from the alphabetized ones). http://codereview.chromium.org/501073/diff/1/7#newcode11 chrome/browser/cocoa/bookmark_groups_controller.mm:11: Kill the double-quadruple newlines. http://codereview.chromium.org/501073/diff/1/7#newcode14 chrome/browser/cocoa/bookmark_groups_controller.mm:14: - (void) syncSelection; No space between |(void)| and method name. http://codereview.chromium.org/501073/diff/1/7#newcode25 chrome/browser/cocoa/bookmark_groups_controller.mm:25: // Completely reloads the contents of the table view. Since this is public, you can move this description to the .h file. http://codereview.chromium.org/501073/diff/1/7#newcode28 chrome/browser/cocoa/bookmark_groups_controller.mm:28: BookmarkModel *bookmarkModel = [manager_ bookmarkModel]; Probably |BookmarkModel* bookmarkModel|. http://codereview.chromium.org/501073/diff/1/7#newcode55 chrome/browser/cocoa/bookmark_groups_controller.mm:55: int row = [groupsTable_ selectedRow]; Probably |row| should be declared as an NSInteger. <shrug> http://codereview.chromium.org/501073/diff/1/7#newcode93 chrome/browser/cocoa/bookmark_groups_controller.mm:93: NSBeep(); I'm not sure if we want to hardcode in a beep, but I'm not really sure what the right thing to do is. http://codereview.chromium.org/501073/diff/1/7#newcode98 chrome/browser/cocoa/bookmark_groups_controller.mm:98: //TODO(snej): Should this require confirmation? No, but it should have undo? http://codereview.chromium.org/501073/diff/1/7#newcode124 chrome/browser/cocoa/bookmark_groups_controller.mm:124: { { should go on previous line. http://codereview.chromium.org/501073/diff/1/8 File chrome/browser/cocoa/bookmark_manager_mac.h (right): http://codereview.chromium.org/501073/diff/1/8#newcode5 chrome/browser/cocoa/bookmark_manager_mac.h:5: #import <Cocoa/Cocoa.h> Newline. http://codereview.chromium.org/501073/diff/1/8#newcode26 chrome/browser/cocoa/bookmark_manager_mac.h:26: + (BookmarkManager*)showBookmarkManager:(Profile*)profile; Comments. http://codereview.chromium.org/501073/diff/1/8#newcode33 chrome/browser/cocoa/bookmark_manager_mac.h:33: - (NSImage*) iconForItem:(id)item; No space. Etc.
Not a single unit test in this entire CL? I realize this is supposed to be temporary but if we expect this to be used by users we can't ignore it. http://codereview.chromium.org/501073/diff/1/2 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/501073/diff/1/2#newcode6003 chrome/app/generated_resources.grd:6003: <message name="IDS_SHOW_BOOKMARKS_MAC" desc="The Mac menu item to show bookmarks in the window menu."> Perhaps use IDS_BOOKMARKS_TITLE in the nib in the short term so no translation needed? http://codereview.chromium.org/501073/diff/1/6 File chrome/browser/cocoa/bookmark_groups_controller.h (right): http://codereview.chromium.org/501073/diff/1/6#newcode17 chrome/browser/cocoa/bookmark_groups_controller.h:17: NSMutableArray* groups_; Implicitly strong, and "@property copied", but you have no dealloc so it'll get leaked if ever set. Similar problem with selectedGroup. http://codereview.chromium.org/501073/diff/1/6#newcode17 chrome/browser/cocoa/bookmark_groups_controller.h:17: NSMutableArray* groups_; Specify types of this array. "Array of things grouped" isn't helpful. http://codereview.chromium.org/501073/diff/1/6#newcode18 chrome/browser/cocoa/bookmark_groups_controller.h:18: id selectedGroup_; Why id and not, say, NSObject*, or id<NSObject>? You retain/release these. http://codereview.chromium.org/501073/diff/1/6#newcode26 chrome/browser/cocoa/bookmark_groups_controller.h:26: - (void) reload; Ditto trung's comments (space between rtn and name; comments on each method) http://codereview.chromium.org/501073/diff/1/7 File chrome/browser/cocoa/bookmark_groups_controller.mm (right): http://codereview.chromium.org/501073/diff/1/7#newcode13 chrome/browser/cocoa/bookmark_groups_controller.mm:13: @interface BookmarkGroupsController () () --> (PrivateAPI) ? See other use in this directory for local conventions. http://codereview.chromium.org/501073/diff/1/7#newcode14 chrome/browser/cocoa/bookmark_groups_controller.mm:14: - (void) syncSelection; Description of syncSelection? http://codereview.chromium.org/501073/diff/1/7#newcode15 chrome/browser/cocoa/bookmark_groups_controller.mm:15: - (void) reload; You define "reload" in the header so no need to do it here as well. http://codereview.chromium.org/501073/diff/1/7#newcode20 chrome/browser/cocoa/bookmark_groups_controller.mm:20: lots of spaces here which doesn't match local convention http://codereview.chromium.org/501073/diff/1/7#newcode33 chrome/browser/cocoa/bookmark_groups_controller.mm:33: const BookmarkNode* child = node->GetChild(i); Looks like non-folder bookmarks inside "Other Bookmarks" (at the top level) won't show up in the manager. Why? Wouldn't the user want to arrange them as well? http://codereview.chromium.org/501073/diff/1/7#newcode59 chrome/browser/cocoa/bookmark_groups_controller.mm:59: if (![item isKindOfClass:[NSValue class]]) In the header you call it "id" but here you have internal knowledge (e.g. make sure it's an NSValue). Perhaps move the isKindOfClass check to the manager_ object itself? http://codereview.chromium.org/501073/diff/1/7#newcode70 chrome/browser/cocoa/bookmark_groups_controller.mm:70: - (void) syncSelection { (void) syncSelection --> (void)syncSelection http://codereview.chromium.org/501073/diff/1/7#newcode93 chrome/browser/cocoa/bookmark_groups_controller.mm:93: NSBeep(); On 2009/12/17 01:40:37, viettrungluu wrote: > I'm not sure if we want to hardcode in a beep, but I'm not really sure what the > right thing to do is. Perhaps the right thing to do is validatedUserInterfaceItem so you can't execute the delete command if nothing is selected. http://codereview.chromium.org/501073/diff/1/7#newcode142 chrome/browser/cocoa/bookmark_groups_controller.mm:142: if ([event keyCode] == 51) // Delete key Why did you chose to do this instead of setKeyEquivalent:? http://codereview.chromium.org/501073/diff/1/8 File chrome/browser/cocoa/bookmark_manager_mac.h (right): http://codereview.chromium.org/501073/diff/1/8#newcode1 chrome/browser/cocoa/bookmark_manager_mac.h:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. There is no need to name things blah_blah_MAC.h when the file is in browser/cocoa, since by definition all files in there are mac files. In addition this filename doesn't match the classname. http://codereview.chromium.org/501073/diff/1/8#newcode14 chrome/browser/cocoa/bookmark_manager_mac.h:14: @interface BookmarkManager : NSWindowController { The convention in browser/cocoa is to name controllers "controller". E.g. "BookmarkManagerController". http://codereview.chromium.org/501073/diff/1/8#newcode21 chrome/browser/cocoa/bookmark_manager_mac.h:21: Profile* profile_; label as weak http://codereview.chromium.org/501073/diff/1/8#newcode23 chrome/browser/cocoa/bookmark_manager_mac.h:23: NSMapTable* nodeMap_; nodeMap_ strong but never released so leaked http://codereview.chromium.org/501073/diff/1/9 File chrome/browser/cocoa/bookmark_manager_mac.mm (right): http://codereview.chromium.org/501073/diff/1/9#newcode25 chrome/browser/cocoa/bookmark_manager_mac.mm:25: @interface BookmarkManager () (PrivateAPI) http://codereview.chromium.org/501073/diff/1/9#newcode80 chrome/browser/cocoa/bookmark_manager_mac.mm:80: BookmarkManager* manager_; label as weak http://codereview.chromium.org/501073/diff/1/9#newcode88 chrome/browser/cocoa/bookmark_manager_mac.mm:88: static NSImage *sFolderIcon; Why globals and not members? Likely cached for other reasons; even if not this doesn't really seem warranted. http://codereview.chromium.org/501073/diff/1/9#newcode94 chrome/browser/cocoa/bookmark_manager_mac.mm:94: self = [super initWithWindowNibName:@"BookmarkManager"]; Use initWithWindowNibPath:: instead of initWithWindowNibName: so we can override it in a unit test. See other examples in this directory. http://codereview.chromium.org/501073/diff/1/9#newcode113 chrome/browser/cocoa/bookmark_manager_mac.mm:113: if (observer_) { some leaks to handle here (see header comments) http://codereview.chromium.org/501073/diff/1/9#newcode147 chrome/browser/cocoa/bookmark_manager_mac.mm:147: nodeMap_ = [[NSMapTable alloc] initWithKeyOptions:NSPointerFunctionsOpaqueMemory | >80s http://codereview.chromium.org/501073/diff/1/9#newcode150 chrome/browser/cocoa/bookmark_manager_mac.mm:150: capacity:500]; capacity should be known at this point; no need to guess 500 http://codereview.chromium.org/501073/diff/1/9#newcode152 chrome/browser/cocoa/bookmark_manager_mac.mm:152: NSValue *item = (NSValue*) NSMapGet(nodeMap_, node); NSValue* item = (NSValue*)NSMapGet(...); 2 space fixes there http://codereview.chromium.org/501073/diff/1/9#newcode181 chrome/browser/cocoa/bookmark_manager_mac.mm:181: const BookmarkNode* node = [self nodeFromItem: item]; extra space in there http://codereview.chromium.org/501073/diff/1/9#newcode210 chrome/browser/cocoa/bookmark_manager_mac.mm:210: - (void) windowWillClose:(NSNotification*)n { extra space in there (a few in this file) http://codereview.chromium.org/501073/diff/1/10 File chrome/browser/cocoa/bookmark_tree_controller.h (right): http://codereview.chromium.org/501073/diff/1/10#newcode22 chrome/browser/cocoa/bookmark_tree_controller.h:22: @property (retain) NSArray* selectedItems; retained so we need a dealloc to release them else leaked if you make a selection then close the bookmark manager. http://codereview.chromium.org/501073/diff/1/10#newcode35 chrome/browser/cocoa/bookmark_tree_controller.h:35: // (These are implemented in bookmark_tree_controller+paste.mm.) You mean bookmark_tree_controller_pasteboard.mm http://codereview.chromium.org/501073/diff/1/11 File chrome/browser/cocoa/bookmark_tree_controller.mm (right): http://codereview.chromium.org/501073/diff/1/11#newcode25 chrome/browser/cocoa/bookmark_tree_controller.mm:25: [outline_ setTarget:self]; Why isn't target hooked up in the nib? http://codereview.chromium.org/501073/diff/1/11#newcode104 chrome/browser/cocoa/bookmark_tree_controller.mm:104: const BookmarkNode* node = [manager_ nodeFromItem:[outline_ itemAtRow:row]]; >80 http://codereview.chromium.org/501073/diff/1/11#newcode133 chrome/browser/cocoa/bookmark_tree_controller.mm:133: const BookmarkNode *node = [self nodeFromItem:item]; lots of "Foo *" instead of "Foo*" in here http://codereview.chromium.org/501073/diff/1/11#newcode194 chrome/browser/cocoa/bookmark_tree_controller.mm:194: //TODO(snej): Uncomment this once SetURL exists (bug 10603). use the easy work-around? (remove node, add new one) http://codereview.chromium.org/501073/diff/1/11#newcode249 chrome/browser/cocoa/bookmark_tree_controller.mm:249: if ([event keyCode] == 51) // Delete key ditto comment from other file http://codereview.chromium.org/501073/diff/1/12 File chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm (right): http://codereview.chromium.org/501073/diff/1/12#newcode25 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:25: static NSArray* kDroppableTypes; How about member instead of global? In particular it then won't show up as a valgrind leak when running your unit tests. http://codereview.chromium.org/501073/diff/1/12#newcode30 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:30: if( ! kDroppableTypes ) extra spaces http://codereview.chromium.org/501073/diff/1/12#newcode76 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:76: [urlStrings addObject:base::SysUTF8ToNSString(node->GetURL().possibly_invalid_spec())]; >80 http://codereview.chromium.org/501073/diff/1/12#newcode153 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:153: NSBeep(); DCHECK() instead of NSBeep()? http://codereview.chromium.org/501073/diff/1/12#newcode172 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:172: const NSString* kTitleKey = @"Title"; kTitleKey doesn't really seem like a unique enough name for your globals. Ditto for a few others. Perhaps place in anon namespace? http://codereview.chromium.org/501073/diff/1/12#newcode392 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:392: NSBeep(); DCHECK http://codereview.chromium.org/501073/diff/1/16 File third_party/apple/ImageAndTextCell.h (right): http://codereview.chromium.org/501073/diff/1/16#newcode1 third_party/apple/ImageAndTextCell.h:1: /* Neither this nor .m added to any gyp file in this CL
http://codereview.chromium.org/501073/diff/1/16 File third_party/apple/ImageAndTextCell.h (right): http://codereview.chromium.org/501073/diff/1/16#newcode1 third_party/apple/ImageAndTextCell.h:1: /* On 2009/12/17 04:31:14, John Grabowski wrote: > Neither this nor .m added to any gyp file in this CL Also, if you add this, you need to add a README.chromium and a LICENSE file, see http://crbug.com/29334
On 2009/12/18 21:49:37, Nico wrote: > http://codereview.chromium.org/501073/diff/1/16 > File third_party/apple/ImageAndTextCell.h (right): > > http://codereview.chromium.org/501073/diff/1/16#newcode1 > third_party/apple/ImageAndTextCell.h:1: /* > On 2009/12/17 04:31:14, John Grabowski wrote: > > Neither this nor .m added to any gyp file in this CL > > Also, if you add this, you need to add a README.chromium and a LICENSE file, see > http://crbug.com/29334 Drive-by comment: You can use NSButtonCell instead and get the same effect as ImageAndTextCell. For an example, see keyword_editor_cocoa_controller.mm.
I think he can't cause he needs an editable cell. Sent from my mobile computing device, which nearly made me write “edible cell”. On Dec 22, 2009 7:28 AM, <rsesek@chromium.org> wrote: On 2009/12/18 21:49:37, Nico wrote: > > http://codereview.chromium.org/501073/diff/1/16 > File third... Drive-by comment: You can use NSButtonCell instead and get the same effect as ImageAndTextCell. For an example, see keyword_editor_cocoa_controller.mm. http://codereview.chromium.org/501073
I think he can't cause he needs an editable cell. Sent from my mobile computing device, which nearly made me write “edible cell”. On Dec 22, 2009 7:28 AM, <rsesek@chromium.org> wrote: On 2009/12/18 21:49:37, Nico wrote: > > http://codereview.chromium.org/501073/diff/1/16 > File third... Drive-by comment: You can use NSButtonCell instead and get the same effect as ImageAndTextCell. For an example, see keyword_editor_cocoa_controller.mm. http://codereview.chromium.org/501073
Sorry for the delay responding, I've been out with a bad cold since Thursday. I'm about to upload a new patch that addresses most of the comments. I'm aware this needs unit tests -- I'd like to ask for some help getting started because I haven't written AppKit-level unit tests before, so I'm not sure how to make things testable. http://codereview.chromium.org/501073/diff/1/2 File chrome/app/generated_resources.grd (right): http://codereview.chromium.org/501073/diff/1/2#newcode6003 chrome/app/generated_resources.grd:6003: <message name="IDS_SHOW_BOOKMARKS_MAC" desc="The Mac menu item to show bookmarks in the window menu."> On 2009/12/17 04:31:14, John Grabowski wrote: > Perhaps use IDS_BOOKMARKS_TITLE in the nib in the short term so no translation > needed? Good point; I'll do that. Also, I just realized that the title of the menu should be more descriptive, like "Bookmark Manager", so I'll change that here. (I originally put it in the Window menu, where "Bookmarks" was an appropriate title, but then moved it.) http://codereview.chromium.org/501073/diff/1/6 File chrome/browser/cocoa/bookmark_groups_controller.h (right): http://codereview.chromium.org/501073/diff/1/6#newcode6 chrome/browser/cocoa/bookmark_groups_controller.h:6: @class BookmarkManager; On 2009/12/17 01:40:37, viettrungluu wrote: > Blank line between the #import and the @class, please. Done. http://codereview.chromium.org/501073/diff/1/6#newcode9 chrome/browser/cocoa/bookmark_groups_controller.h:9: On 2009/12/17 01:40:37, viettrungluu wrote: > Unless the file is big and hairy, we can probably do without the double > linebreaks. I feel strongly about them, for delimiting different sections of the file. AFAICT the style guide just says "don't put more than one or two blank lines between functions" and calls it a "principle more than a rule". http://codereview.chromium.org/501073/diff/1/6#newcode11 chrome/browser/cocoa/bookmark_groups_controller.h:11: // Controller for the bookmark group list (the left pane). On 2009/12/17 01:40:37, viettrungluu wrote: > The left pane of what? Of the window. http://codereview.chromium.org/501073/diff/1/6#newcode14 chrome/browser/cocoa/bookmark_groups_controller.h:14: IBOutlet NSTableView *groupsTable_; On 2009/12/17 01:40:37, viettrungluu wrote: > Probably |NSTableView* groupsTable_| (* attached to the type). We can accept > either, but consistency within each file is key (and in our code we nearly > universally attach the * to the type). Sorry, thought I had already done a search-replace to find these, but I hadn't. I've been attaching the * to the name ever since I read K&R in 1981 and it's hard to switch. http://codereview.chromium.org/501073/diff/1/6#newcode17 chrome/browser/cocoa/bookmark_groups_controller.h:17: NSMutableArray* groups_; On 2009/12/17 04:31:14, John Grabowski wrote: > Implicitly strong, and "@property copied", but you have no dealloc so it'll get > leaked if ever set. Similar problem with selectedGroup. Done. http://codereview.chromium.org/501073/diff/1/6#newcode18 chrome/browser/cocoa/bookmark_groups_controller.h:18: id selectedGroup_; On 2009/12/17 04:31:14, John Grabowski wrote: > Why id and not, say, NSObject*, or id<NSObject>? > You retain/release these. 'id' is idiomatic in Obj-C for a arbitrary/unknown type; 'NSObject*' is not used. (Look at Apple's framework headers for example.) http://codereview.chromium.org/501073/diff/1/6#newcode26 chrome/browser/cocoa/bookmark_groups_controller.h:26: - (void) reload; On 2009/12/17 01:40:37, viettrungluu wrote: > |(void)reload| (no space). OK. (Sorry, Google's Obj-C spacing rules are really hard for me to get used to, they're the opposite of what I normally do.) http://codereview.chromium.org/501073/diff/1/6#newcode31 chrome/browser/cocoa/bookmark_groups_controller.h:31: On 2009/12/17 01:40:37, viettrungluu wrote: > Here too. I use double blank lines between multiple classes in a file. http://codereview.chromium.org/501073/diff/1/7 File chrome/browser/cocoa/bookmark_groups_controller.mm (right): http://codereview.chromium.org/501073/diff/1/7#newcode5 chrome/browser/cocoa/bookmark_groups_controller.mm:5: #import "chrome/browser/cocoa/bookmark_groups_controller.h" On 2009/12/17 01:40:37, viettrungluu wrote: > Probably a newline after this (to separate it from the alphabetized ones). Done. http://codereview.chromium.org/501073/diff/1/7#newcode13 chrome/browser/cocoa/bookmark_groups_controller.mm:13: @interface BookmarkGroupsController () On 2009/12/17 04:31:14, John Grabowski wrote: > () --> (PrivateAPI) ? See other use in this directory for local conventions. Class continuations (unnamed category) are the best way to declare private API in ObjC2. Unlike the old named-category approach it will detect if you forget to implement one of the private methods; this was a frequent source of runtime errors. Also, property declarations can _only_ be amended inside a continuation, not a named category. http://codereview.chromium.org/501073/diff/1/7#newcode14 chrome/browser/cocoa/bookmark_groups_controller.mm:14: - (void) syncSelection; On 2009/12/17 01:40:37, viettrungluu wrote: > No space between |(void)| and method name. Done. http://codereview.chromium.org/501073/diff/1/7#newcode14 chrome/browser/cocoa/bookmark_groups_controller.mm:14: - (void) syncSelection; On 2009/12/17 01:40:37, viettrungluu wrote: > No space between |(void)| and method name. Done. http://codereview.chromium.org/501073/diff/1/7#newcode15 chrome/browser/cocoa/bookmark_groups_controller.mm:15: - (void) reload; On 2009/12/17 04:31:14, John Grabowski wrote: > You define "reload" in the header so no need to do it here as well. Done. http://codereview.chromium.org/501073/diff/1/7#newcode20 chrome/browser/cocoa/bookmark_groups_controller.mm:20: On 2009/12/17 04:31:14, John Grabowski wrote: > lots > of > spaces > here > which > doesn't > match > local > convention Removed some. http://codereview.chromium.org/501073/diff/1/7#newcode25 chrome/browser/cocoa/bookmark_groups_controller.mm:25: // Completely reloads the contents of the table view. On 2009/12/17 01:40:37, viettrungluu wrote: > Since this is public, you can move this description to the .h file. OK, I've added descriptions to the .h. I prefer to leave comments here too to visually separate the methods. http://codereview.chromium.org/501073/diff/1/7#newcode28 chrome/browser/cocoa/bookmark_groups_controller.mm:28: BookmarkModel *bookmarkModel = [manager_ bookmarkModel]; On 2009/12/17 01:40:37, viettrungluu wrote: > Probably |BookmarkModel* bookmarkModel|. Done. http://codereview.chromium.org/501073/diff/1/7#newcode33 chrome/browser/cocoa/bookmark_groups_controller.mm:33: const BookmarkNode* child = node->GetChild(i); On 2009/12/17 04:31:14, John Grabowski wrote: > Looks like non-folder bookmarks inside "Other Bookmarks" (at the top level) > won't show up in the manager. Why? Wouldn't the user want to arrange them as > well? Yeah, I have an action item to ask about that. My own Other collection only has folders inside it, but I don't know if that's always the case or if it only happens when bookmarks are imported from Safari. At a UI level it's kind of messy if the pane contains a mixture of groups and URLs, but if the app allows you to do that there's probably no good way around it... http://codereview.chromium.org/501073/diff/1/7#newcode55 chrome/browser/cocoa/bookmark_groups_controller.mm:55: int row = [groupsTable_ selectedRow]; On 2009/12/17 01:40:37, viettrungluu wrote: > Probably |row| should be declared as an NSInteger. <shrug> Done. http://codereview.chromium.org/501073/diff/1/7#newcode59 chrome/browser/cocoa/bookmark_groups_controller.mm:59: if (![item isKindOfClass:[NSValue class]]) On 2009/12/17 04:31:14, John Grabowski wrote: > In the header you call it "id" but here you have internal knowledge (e.g. make > sure it's an NSValue). Perhaps move the isKindOfClass check to the manager_ > object itself? You're right, this bit is accidentally left over from an earlier design; I should have deleted it already. http://codereview.chromium.org/501073/diff/1/7#newcode70 chrome/browser/cocoa/bookmark_groups_controller.mm:70: - (void) syncSelection { On 2009/12/17 04:31:14, John Grabowski wrote: > (void) syncSelection --> (void)syncSelection Done. http://codereview.chromium.org/501073/diff/1/7#newcode93 chrome/browser/cocoa/bookmark_groups_controller.mm:93: NSBeep(); On 2009/12/17 01:40:37, viettrungluu wrote: > I'm not sure if we want to hardcode in a beep, but I'm not really sure what the > right thing to do is. It's the correct response if the keystroke isn't handled. http://codereview.chromium.org/501073/diff/1/7#newcode93 chrome/browser/cocoa/bookmark_groups_controller.mm:93: NSBeep(); On 2009/12/17 04:31:14, John Grabowski wrote: > On 2009/12/17 01:40:37, viettrungluu wrote: > > I'm not sure if we want to hardcode in a beep, but I'm not really sure what > the > > right thing to do is. > > Perhaps the right thing to do is validatedUserInterfaceItem so you can't execute > the delete command if nothing is selected. > This method is also called when the Delete key is pressed, and there's no way to disable that. http://codereview.chromium.org/501073/diff/1/7#newcode98 chrome/browser/cocoa/bookmark_groups_controller.mm:98: //TODO(snej): Should this require confirmation? On 2009/12/17 01:40:37, viettrungluu wrote: > No, but it should have undo? Yeah, you're right. http://codereview.chromium.org/501073/diff/1/7#newcode124 chrome/browser/cocoa/bookmark_groups_controller.mm:124: { On 2009/12/17 01:40:37, viettrungluu wrote: > { should go on previous line. Done. http://codereview.chromium.org/501073/diff/1/7#newcode142 chrome/browser/cocoa/bookmark_groups_controller.mm:142: if ([event keyCode] == 51) // Delete key On 2009/12/17 04:31:14, John Grabowski wrote: > Why did you chose to do this instead of setKeyEquivalent:? setKeyEquivalent is an NSButton method. This is an NSTableView. Also, the delete key has to apply to the focused view, whereas keyEquivalents are per-window. http://codereview.chromium.org/501073/diff/1/8 File chrome/browser/cocoa/bookmark_manager_mac.h (right): http://codereview.chromium.org/501073/diff/1/8#newcode1 chrome/browser/cocoa/bookmark_manager_mac.h:1: // Copyright (c) 2009 The Chromium Authors. All rights reserved. On 2009/12/17 04:31:14, John Grabowski wrote: > There is no need to name things blah_blah_MAC.h when the file is in > browser/cocoa, since by definition all files in there are mac files. In > addition this filename doesn't match the classname. There's already a bookmark_manager.h in the tree, and I am extremely wary about duplicate header filenames. I looked to see what other platforms did about this, and saw that there's a "bookmark_manager_gtk.h" so I followed that convention. Who's right here? http://codereview.chromium.org/501073/diff/1/8#newcode5 chrome/browser/cocoa/bookmark_manager_mac.h:5: #import <Cocoa/Cocoa.h> On 2009/12/17 01:40:37, viettrungluu wrote: > Newline. Done. http://codereview.chromium.org/501073/diff/1/8#newcode14 chrome/browser/cocoa/bookmark_manager_mac.h:14: @interface BookmarkManager : NSWindowController { On 2009/12/17 04:31:14, John Grabowski wrote: > The convention in browser/cocoa is to name controllers "controller". > E.g. "BookmarkManagerController". > Done. (That fixes the filename issue as a side effect too.) http://codereview.chromium.org/501073/diff/1/8#newcode21 chrome/browser/cocoa/bookmark_manager_mac.h:21: Profile* profile_; On 2009/12/17 04:31:14, John Grabowski wrote: > label as weak Done. http://codereview.chromium.org/501073/diff/1/8#newcode23 chrome/browser/cocoa/bookmark_manager_mac.h:23: NSMapTable* nodeMap_; On 2009/12/17 04:31:14, John Grabowski wrote: > nodeMap_ strong but never released so leaked Weird -- must've lost the release during some refactoring. Done. http://codereview.chromium.org/501073/diff/1/8#newcode26 chrome/browser/cocoa/bookmark_manager_mac.h:26: + (BookmarkManager*)showBookmarkManager:(Profile*)profile; On 2009/12/17 01:40:37, viettrungluu wrote: > Comments. Done. http://codereview.chromium.org/501073/diff/1/8#newcode33 chrome/browser/cocoa/bookmark_manager_mac.h:33: - (NSImage*) iconForItem:(id)item; On 2009/12/17 01:40:37, viettrungluu wrote: > No space. > > Etc. Done. http://codereview.chromium.org/501073/diff/1/9 File chrome/browser/cocoa/bookmark_manager_mac.mm (right): http://codereview.chromium.org/501073/diff/1/9#newcode25 chrome/browser/cocoa/bookmark_manager_mac.mm:25: @interface BookmarkManager () On 2009/12/17 04:31:14, John Grabowski wrote: > (PrivateAPI) See comment in previous file -- class continuation is better than named category for this. http://codereview.chromium.org/501073/diff/1/9#newcode80 chrome/browser/cocoa/bookmark_manager_mac.mm:80: BookmarkManager* manager_; On 2009/12/17 04:31:14, John Grabowski wrote: > label as weak Done. http://codereview.chromium.org/501073/diff/1/9#newcode88 chrome/browser/cocoa/bookmark_manager_mac.mm:88: static NSImage *sFolderIcon; On 2009/12/17 04:31:14, John Grabowski wrote: > Why globals and not members? Likely cached for other reasons; even if not this > doesn't really seem warranted. Done. http://codereview.chromium.org/501073/diff/1/9#newcode94 chrome/browser/cocoa/bookmark_manager_mac.mm:94: self = [super initWithWindowNibName:@"BookmarkManager"]; On 2009/12/17 04:31:14, John Grabowski wrote: > Use initWithWindowNibPath:: instead of initWithWindowNibName: so we can override > it in a unit test. See other examples in this directory. > Done. http://codereview.chromium.org/501073/diff/1/9#newcode113 chrome/browser/cocoa/bookmark_manager_mac.mm:113: if (observer_) { On 2009/12/17 04:31:14, John Grabowski wrote: > some leaks to handle here (see header comments) Done. http://codereview.chromium.org/501073/diff/1/9#newcode147 chrome/browser/cocoa/bookmark_manager_mac.mm:147: nodeMap_ = [[NSMapTable alloc] initWithKeyOptions:NSPointerFunctionsOpaqueMemory | On 2009/12/17 04:31:14, John Grabowski wrote: > >80s Done. http://codereview.chromium.org/501073/diff/1/9#newcode150 chrome/browser/cocoa/bookmark_manager_mac.mm:150: capacity:500]; On 2009/12/17 04:31:14, John Grabowski wrote: > capacity should be known at this point; no need to guess 500 It's dynamic: for example the user might drag in from another browser's bookmark list. Reserving 2k (or 4k) bytes to allow for this seems reasonable. Besides, there's no BookmarkModel method to return the total number of nodes (just a vector of URLs, which doesn't count the folders.) http://codereview.chromium.org/501073/diff/1/9#newcode152 chrome/browser/cocoa/bookmark_manager_mac.mm:152: NSValue *item = (NSValue*) NSMapGet(nodeMap_, node); On 2009/12/17 04:31:14, John Grabowski wrote: > NSValue* item = (NSValue*)NSMapGet(...); > 2 space fixes there > Done. http://codereview.chromium.org/501073/diff/1/9#newcode181 chrome/browser/cocoa/bookmark_manager_mac.mm:181: const BookmarkNode* node = [self nodeFromItem: item]; On 2009/12/17 04:31:14, John Grabowski wrote: > extra space in there Done. http://codereview.chromium.org/501073/diff/1/9#newcode210 chrome/browser/cocoa/bookmark_manager_mac.mm:210: - (void) windowWillClose:(NSNotification*)n { On 2009/12/17 04:31:14, John Grabowski wrote: > extra space in there (a few in this file) Done. http://codereview.chromium.org/501073/diff/1/10 File chrome/browser/cocoa/bookmark_tree_controller.h (right): http://codereview.chromium.org/501073/diff/1/10#newcode22 chrome/browser/cocoa/bookmark_tree_controller.h:22: @property (retain) NSArray* selectedItems; On 2009/12/17 04:31:14, John Grabowski wrote: > retained so we need a dealloc to release them else leaked if you make a > selection then close the bookmark manager. Um, no; there isn't an ivar that stores this; the getter creates the array on the fly. See implementation. http://codereview.chromium.org/501073/diff/1/10#newcode35 chrome/browser/cocoa/bookmark_tree_controller.h:35: // (These are implemented in bookmark_tree_controller+paste.mm.) On 2009/12/17 04:31:14, John Grabowski wrote: > You mean bookmark_tree_controller_pasteboard.mm > > Done. http://codereview.chromium.org/501073/diff/1/11 File chrome/browser/cocoa/bookmark_tree_controller.mm (right): http://codereview.chromium.org/501073/diff/1/11#newcode25 chrome/browser/cocoa/bookmark_tree_controller.mm:25: [outline_ setTarget:self]; On 2009/12/17 04:31:14, John Grabowski wrote: > Why isn't target hooked up in the nib? Because there's no action, and you can't set one without the other. For some reason IB's never allowed you to set the doubleAction. http://codereview.chromium.org/501073/diff/1/11#newcode104 chrome/browser/cocoa/bookmark_tree_controller.mm:104: const BookmarkNode* node = [manager_ nodeFromItem:[outline_ itemAtRow:row]]; On 2009/12/17 04:31:14, John Grabowski wrote: > >80 Done. http://codereview.chromium.org/501073/diff/1/11#newcode133 chrome/browser/cocoa/bookmark_tree_controller.mm:133: const BookmarkNode *node = [self nodeFromItem:item]; On 2009/12/17 04:31:14, John Grabowski wrote: > lots of "Foo *" instead of "Foo*" in here > Done. http://codereview.chromium.org/501073/diff/1/11#newcode194 chrome/browser/cocoa/bookmark_tree_controller.mm:194: //TODO(snej): Uncomment this once SetURL exists (bug 10603). On 2009/12/17 04:31:14, John Grabowski wrote: > use the easy work-around? (remove node, add new one) Either way. I'll update the TODO comment to incorporate that. http://codereview.chromium.org/501073/diff/1/11#newcode249 chrome/browser/cocoa/bookmark_tree_controller.mm:249: if ([event keyCode] == 51) // Delete key On 2009/12/17 04:31:14, John Grabowski wrote: > ditto comment from other file Ditto answer from other file :-) http://codereview.chromium.org/501073/diff/1/12 File chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm (right): http://codereview.chromium.org/501073/diff/1/12#newcode25 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:25: static NSArray* kDroppableTypes; On 2009/12/17 04:31:14, John Grabowski wrote: > How about member instead of global? In particular it then won't show up as a > valgrind leak when running your unit tests. It's conceptually a constant, not a piece of state. But if valgrind's going to complain, I can change it. (I found a way to do it without needing a variable.) http://codereview.chromium.org/501073/diff/1/12#newcode30 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:30: if( ! kDroppableTypes ) On 2009/12/17 04:31:14, John Grabowski wrote: > extra spaces Done. http://codereview.chromium.org/501073/diff/1/12#newcode76 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:76: [urlStrings addObject:base::SysUTF8ToNSString(node->GetURL().possibly_invalid_spec())]; On 2009/12/17 04:31:14, John Grabowski wrote: > >80 Done. http://codereview.chromium.org/501073/diff/1/12#newcode153 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:153: NSBeep(); On 2009/12/17 04:31:14, John Grabowski wrote: > DCHECK() instead of NSBeep()? > No, if something's selected that can't be written to the pasteboard I'd rather the app beep than crash. http://codereview.chromium.org/501073/diff/1/12#newcode172 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:172: const NSString* kTitleKey = @"Title"; On 2009/12/17 04:31:14, John Grabowski wrote: > kTitleKey doesn't really seem like a unique enough name for your globals. Ditto > for a few others. Perhaps place in anon namespace? Good point. I originally used #defines then changed them to consts. I'll make them static. http://codereview.chromium.org/501073/diff/1/12#newcode392 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:392: NSBeep(); On 2009/12/17 04:31:14, John Grabowski wrote: > DCHECK No, this is a possible runtime error condition, if the data put on the pasteboard by another app isn't valid. It's not like an assertion failure. http://codereview.chromium.org/501073/diff/1/16 File third_party/apple/ImageAndTextCell.h (right): http://codereview.chromium.org/501073/diff/1/16#newcode1 third_party/apple/ImageAndTextCell.h:1: /* On 2009/12/17 04:31:14, John Grabowski wrote: > Neither this nor .m added to any gyp file in this CL Yeah, discovered that soon afterwards. Fixed. http://codereview.chromium.org/501073/diff/1/16#newcode1 third_party/apple/ImageAndTextCell.h:1: /* On 2009/12/18 21:49:39, Nico wrote: > On 2009/12/17 04:31:14, John Grabowski wrote: > > Neither this nor .m added to any gyp file in this CL > > Also, if you add this, you need to add a README.chromium and a LICENSE file, see > http://crbug.com/29334 Tnanks. Added a README.chromium; the sample code already has its own license inline so I don't think a separate file is necessary.
http://codereview.chromium.org/501073/diff/1/16 File third_party/apple/ImageAndTextCell.h (right): http://codereview.chromium.org/501073/diff/1/16#newcode1 third_party/apple/ImageAndTextCell.h:1: /* On 2009/12/22 17:59:51, Jens Alfke wrote: > On 2009/12/18 21:49:39, Nico wrote: > > On 2009/12/17 04:31:14, John Grabowski wrote: > > > Neither this nor .m added to any gyp file in this CL > > > > Also, if you add this, you need to add a README.chromium and a LICENSE file, > see > > http://crbug.com/29334 > > Tnanks. Added a README.chromium; the sample code already has its own license > inline so I don't think a separate file is necessary. It is necessary, see http://wiki.corp.google.com/twiki/bin/view/Main/ThirdParty#Document_the_Code_... . Looks like we have no version of this on the chromium wiki :-( You can copy the license from the source file and put this into a new file called "license".
> > > I'm aware this needs unit tests -- I'd like to ask for some help getting > started > because I haven't written AppKit-level unit tests before, so I'm not sure > how to > make things testable. > > I'm not sure what your unit test background is so I'm not sure where to start. Presumably you've done the unit test codelab and are familiar with basic concepts (e.g. functional programming techniques to make testing easier, mock objects, dependancy injection). About 70% of the files in chrome/browser/cocoa have unit test files so there should be plenty of examples. One of the most minimal is background_gradient_view_unittest.mm. All this one does is draw the custom view. Arguably a test which doesn't confirm output is "cheating" (e.g. on code coverage). However, all our unit tests run under valgrind, so even a test like this will be confirmed to have no memory leaks, stomps, or crashes. One of the largest is bookmark_bar_controller_unittest.mm which contains a number of explicitly mocked objects. You can check the waterfall to see the code coverage of this test; for bookmark_bar_controller.mm it is about 75%. http://build.chromium.org/buildbot/coverage/mac-debug/34668/CHROMIUM/chrome/b... We have GUnit, GMock, and OCMock in our tree. jrg > http://codereview.chromium.org/501073 >
Still reading (esp. bookmark_tree_controller_pasteboard.mm and at the "big picture"), but I may as well send this out now.... http://codereview.chromium.org/501073/diff/8001/38 File chrome/browser/cocoa/bookmark_groups_controller.h (right): http://codereview.chromium.org/501073/diff/8001/38#newcode18 chrome/browser/cocoa/bookmark_groups_controller.h:18: NSMutableArray* groups_; // array of node items ('id's) Can you make these scoped_nsobject's? Looks like you can, to me. http://codereview.chromium.org/501073/diff/8001/38#newcode24 chrome/browser/cocoa/bookmark_groups_controller.h:24: @property (copy) NSArray* groups; ... though then you'd have to implement the getters/setters yourself. Hmmm. http://codereview.chromium.org/501073/diff/8001/38#newcode34 chrome/browser/cocoa/bookmark_groups_controller.h:34: // Called by the BookmarkManagerController to notify that the data model's changed. 80 columns. http://codereview.chromium.org/501073/diff/8001/39 File chrome/browser/cocoa/bookmark_groups_controller.mm (right): http://codereview.chromium.org/501073/diff/8001/39#newcode24 chrome/browser/cocoa/bookmark_groups_controller.mm:24: [groups_ release]; 2-space indents. http://codereview.chromium.org/501073/diff/8001/39#newcode45 chrome/browser/cocoa/bookmark_groups_controller.mm:45: [self setGroups:groups]; Remove 2-spaces of indent. http://codereview.chromium.org/501073/diff/8001/39#newcode53 chrome/browser/cocoa/bookmark_groups_controller.mm:53: [self reload]; This doesn't seem so efficient, though I don't know about the implications in practice. http://codereview.chromium.org/501073/diff/8001/39#newcode123 chrome/browser/cocoa/bookmark_groups_controller.mm:123: [(ImageAndTextCell*)cell setImage:[manager_ iconForItem:item]]; Unindent 2 spaces. http://codereview.chromium.org/501073/diff/8001/40 File chrome/browser/cocoa/bookmark_manager_controller.h (right): http://codereview.chromium.org/501073/diff/8001/40#newcode23 chrome/browser/cocoa/bookmark_manager_controller.h:23: BookmarkManagerObserver* observer_; These appear to be strong. Please use scoped_nsobject unless there's a compelling reason not to (it'll also get rid of a bunch of code in the -dealloc). http://codereview.chromium.org/501073/diff/8001/41 File chrome/browser/cocoa/bookmark_manager_controller.mm (right): http://codereview.chromium.org/501073/diff/8001/41#newcode10 chrome/browser/cocoa/bookmark_manager_controller.mm:10: #import "chrome/browser/bookmarks/bookmark_model.h" These should be #include? http://codereview.chromium.org/501073/diff/8001/41#newcode36 chrome/browser/cocoa/bookmark_manager_controller.mm:36: public: 1-space indent (I don't make these rules ...). http://codereview.chromium.org/501073/diff/8001/41#newcode37 chrome/browser/cocoa/bookmark_manager_controller.mm:37: BookmarkManagerObserver(BookmarkManagerController* manager) See the C++ style guide. I'm guessing this should be formatted something like: BookmarkManagerObserver(BookmarkManagerController* manager) : manager_(manager) { } http://codereview.chromium.org/501073/diff/8001/41#newcode41 chrome/browser/cocoa/bookmark_manager_controller.mm:41: virtual void Loaded(BookmarkModel* model) { } Could you add a comment saying that it's safe to ignore this since individual BookmarkNodeAdded() notifications will be sent? http://codereview.chromium.org/501073/diff/8001/41#newcode81 chrome/browser/cocoa/bookmark_manager_controller.mm:81: private: 1-space indent. http://codereview.chromium.org/501073/diff/8001/41#newcode93 chrome/browser/cocoa/bookmark_manager_controller.mm:93: NSString* nibPath = [mac_util::MainAppBundle() Could you split/align this differently? (This looks as if "pathForResource:" is aligned with "mac_util:".) http://codereview.chromium.org/501073/diff/8001/41#newcode96 chrome/browser/cocoa/bookmark_manager_controller.mm:96: self = [super initWithWindowNibPath:nibPath owner: self]; No space after : (hard to get used to, I know). http://codereview.chromium.org/501073/diff/8001/41#newcode167 chrome/browser/cocoa/bookmark_manager_controller.mm:167: for (int i = node->GetChildCount() -1 ; i >= 0 ; i--) { Space after the "-" (binary operator, not unary). Probably no space before the ";"s, but I'd have to check the style guide to be sure about that. http://codereview.chromium.org/501073/diff/8001/41#newcode221 chrome/browser/cocoa/bookmark_manager_controller.mm:221: No blank line here. http://codereview.chromium.org/501073/diff/8001/41#newcode238 chrome/browser/cocoa/bookmark_manager_controller.mm:238: if (!node->is_url()) Could you add a DCHECK(node) here? (Possibly also a paranoia-check in the if: |if (!node || !node->is_url())|.) Incidentally, can one (or should one be able to) open a whole folder? http://codereview.chromium.org/501073/diff/8001/43 File chrome/browser/cocoa/bookmark_tree_controller.mm (right): http://codereview.chromium.org/501073/diff/8001/43#newcode93 chrome/browser/cocoa/bookmark_tree_controller.mm:93: if (selectedRows != nil) { This one doesn't beep? It might be better to check for nil-ness and |return|, and unindent the rest. http://codereview.chromium.org/501073/diff/8001/43#newcode166 chrome/browser/cocoa/bookmark_tree_controller.mm:166: if ([ident isEqualToString:@"title"]) { I'm not sure how I feel about these hard-coded identifiers, so I'll let it pass. http://codereview.chromium.org/501073/diff/8001/43#newcode171 chrome/browser/cocoa/bookmark_tree_controller.mm:171: return nil; Should this ever be reached? Maybe it deserves a NOTREACHED()? http://codereview.chromium.org/501073/diff/8001/43#newcode185 chrome/browser/cocoa/bookmark_tree_controller.mm:185: GURL url([value UTF8String]); Probably you should use base::SysNSStringToUTF8(), or so people have told me. http://codereview.chromium.org/501073/diff/8001/44 File chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm (right): http://codereview.chromium.org/501073/diff/8001/44#newcode71 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:71: [urlStrings addObject:base::SysUTF8ToNSString(node->GetURL().possibly_invalid_spec())]; 80 columns. http://codereview.chromium.org/501073/diff/8001/45 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/501073/diff/8001/45#newcode257 chrome/browser/cocoa/browser_window_cocoa.mm:257: [BookmarkManagerController showBookmarkManager: browser_->profile()]; No space after :. http://codereview.chromium.org/501073/diff/8001/50 File third_party/apple/README.chromium (right): http://codereview.chromium.org/501073/diff/8001/50#newcode1 third_party/apple/README.chromium:1: This is Chrome's local copy of Apple sample code, Probably this should be "Chromium" (given that this is README.chromium). http://codereview.chromium.org/501073/diff/8001/50#newcode6 third_party/apple/README.chromium:6: * ImageAndTextCell.h: Changed 'image' property to 'retain' mode to fix a crash. 80 columns?
My second batch of comments.... http://codereview.chromium.org/501073/diff/8001/44 File chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm (right): http://codereview.chromium.org/501073/diff/8001/44#newcode12 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:12: A general FYI: we sometimes use stuff from Mozilla for URL pasteboard stuff (see third_party/mozilla/include/NSPasteboard+Utils.*). I think it'd be good if we could come up with our own stuff which provides a superset of the stuff you do and the stuff provided by the Mozilla code. (My ulterior motive is that the Mozilla code has a large number of deficiencies....) This is a "to do" rather than something which you need to address now. http://codereview.chromium.org/501073/diff/8001/44#newcode13 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:13: // Safari uses this type, though it's not declared in any header. Are these documented anywhere? Could you either provide documentation or a pointer to documentation? Also, I'm not sure why you use #defines here rather than (static, or in an anonymous namespace) consts. http://codereview.chromium.org/501073/diff/8001/44#newcode19 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:19: // Used internally to indentify intra-outline drags. s/indentify/identify Also, will it ever be visible to outside applications? (To avoid potential name conflicts, it might be best to attach "Chrome" or "Chromium" to the the type name.) http://codereview.chromium.org/501073/diff/8001/44#newcode27 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:27: - (void)_initDragging { We don't usually name things beginning with _; maybe just give it a longer, more descriptive name (without initial _) instead? http://codereview.chromium.org/501073/diff/8001/44#newcode39 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:39: id parentItem = [self itemFromNode:parent]; Could you add DCHECK()s to sanity-check childRange.location and childRange.length? http://codereview.chromium.org/501073/diff/8001/44#newcode91 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:91: // Prepare clipboard and declare types: If you just declared a pasteboard and then added types, rather than making an NSMutableArray and removing types. http://codereview.chromium.org/501073/diff/8001/44#newcode109 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:109: // drags aren't recognized by anyone but us. It's not the most likely thing in the world, but will this blow up if you run two instances of Chromium with different profiles, and drag from one bookmark manager to the other? http://codereview.chromium.org/501073/diff/8001/44#newcode165 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:165: static const NSString* kTitleKey = @"Title"; Could you provide a description of what's going on? (I'm repeating myself, I know.) It might be cleaner if you wrapped these in an anonymous namespace rather than using |static|, but it's not a big deal. http://codereview.chromium.org/501073/diff/8001/44#newcode175 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:175: static NSDictionary* makeBookmarkPlistEntry(NSString* name, NSString* urlStr) { It's worth mentioning that |name| may be nil, but not |urlStr| (AFAICT). http://codereview.chromium.org/501073/diff/8001/44#newcode176 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:176: if (!name) { What if |name != nil| but is an empty string? Is this possible? http://codereview.chromium.org/501073/diff/8001/44#newcode193 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:193: if( [type isEqualToString:BookmarkDictionaryListPboardType] ) { Our style guide dictates (for better or worse): if ([type ...]) { (space after |if|, no spaces inside parens). http://codereview.chromium.org/501073/diff/8001/44#newcode197 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:197: } else if( [type isEqualToString:WebURLsWithTitlesPboardType] ) { Here too. http://codereview.chromium.org/501073/diff/8001/44#newcode222 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:222: return [NSArray arrayWithObject:makeBookmarkPlistEntry(urlStr, title)]; urlStr <-> title. http://codereview.chromium.org/501073/diff/8001/44#newcode235 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:235: it < nodes.end(); it++) { ++it, not it++. (Probably |it != nodes.end()| would be more idiomatic than <, but I'm too lazy to figure out if it makes a difference. Probably not in this case.) http://codereview.chromium.org/501073/diff/8001/44#newcode236 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:236: NSAutoreleasePool* pool = [NSAutoreleasePool new]; I don't object, but is there a reason you need/want to do this? Probably worth a comment. http://codereview.chromium.org/501073/diff/8001/44#newcode256 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:256: int i = 0; Probably if |dstIndex| is an NSInteger, so too should |i|. http://codereview.chromium.org/501073/diff/8001/44#newcode265 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:265: dstIndex + i++, I think I'm legally obligated to object to |dstIndex + i++|; better to move the i++ outside the function call. http://codereview.chromium.org/501073/diff/8001/44#newcode275 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:275: dstIndex + i++, Ditto. http://codereview.chromium.org/501073/diff/8001/44#newcode359 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:359: if ([info draggingSource] == outlineView && Okay, I guess it won't blow up. http://codereview.chromium.org/501073/diff/8001/44#newcode380 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:380: NSArray* plist = [self readPropertyListFromPasteboard:[NSPasteboard generalPasteboard]]; 80 columns. http://codereview.chromium.org/501073/diff/8001/44#newcode382 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:382: NSBeep(); Given what you do in |-validateMenuItem:|, do you expect the NSBeep()s (here and elsewhere) to be called? If not, it might be worth a NOTREACHED() or a LOG(WARNING) << "...", or whatever.
Now, to actually respond to some of what you said.... On 2009/12/22 17:59:51, Jens Alfke wrote: > Sorry for the delay responding, I've been out with a bad cold since Thursday. I hope you're feeling better.... > I'm aware this needs unit tests -- I'd like to ask for some help getting started > because I haven't written AppKit-level unit tests before, so I'm not sure how to > make things testable. What I'd like to see (minimally, in an ideal world): - test the various pasteboard routines; for this, you might want to split the code up a bit more so that you can provide the routines with a pasteboard (rather than having stuff access the system pasteboard) - it's hard to test UI in general, but it'd be good to test, e.g., selecting, moving, etc. bookmarks in a wide variety of situations (with various kinds of selections) > > Unless the file is big and hairy, we can probably do without the double > > linebreaks. > > I feel strongly about them, for delimiting different sections of the file. > AFAICT the style guide just says "don't put more than one or two blank lines > between functions" and calls it a "principle more than a rule". Okay, as long as it's not excessive and you're consistent. (Left to my own devices, I'd be inclined to do likewise.) > > Probably |NSTableView* groupsTable_| (* attached to the type). We can accept > > either, but consistency within each file is key (and in our code we nearly > > universally attach the * to the type). > > Sorry, thought I had already done a search-replace to find these, but I hadn't. > I've been attaching the * to the name ever since I read K&R in 1981 and it's > hard to switch. No worries. (K&R were obviously right -- though that doesn't change the reality of our codebase. Really, blame lies with R's syntax for declaring pointers.) > http://codereview.chromium.org/501073/diff/1/12#newcode153 > chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:153: NSBeep(); > On 2009/12/17 04:31:14, John Grabowski wrote: > > DCHECK() instead of NSBeep()? > > No, if something's selected that can't be written to the pasteboard I'd rather > the app beep than crash. It'll only crash on a debug build. Thanks for taking care of (or at least responding to) the things pointed out. (Sorry also about the torrent of style complaints, but those are the easiest to spot. I hope you'll bear with me....) I still have to look over things more carefully from a global standpoint. I'd also like to see some unit tests -- having just a decent amount of code running in a basic unit test would be helpful since, as John pointed out, we do run the tests through valgrind.
http://codereview.chromium.org/501073/diff/8001/38 File chrome/browser/cocoa/bookmark_groups_controller.h (right): http://codereview.chromium.org/501073/diff/8001/38#newcode24 chrome/browser/cocoa/bookmark_groups_controller.h:24: @property (copy) NSArray* groups; On 2009/12/22 20:02:43, viettrungluu wrote: > ... though then you'd have to implement the getters/setters yourself. Hmmm. Yeah, I'd rather do it the old fashioned way and be able to synthesize the accessors. It just takes one release call in the dealloc. http://codereview.chromium.org/501073/diff/8001/38#newcode34 chrome/browser/cocoa/bookmark_groups_controller.h:34: // Called by the BookmarkManagerController to notify that the data model's changed. On 2009/12/22 20:02:43, viettrungluu wrote: > 80 columns. Done. http://codereview.chromium.org/501073/diff/8001/39 File chrome/browser/cocoa/bookmark_groups_controller.mm (right): http://codereview.chromium.org/501073/diff/8001/39#newcode24 chrome/browser/cocoa/bookmark_groups_controller.mm:24: [groups_ release]; On 2009/12/22 20:02:43, viettrungluu wrote: > 2-space indents. Aw crap. I blame Xcode. Fixed. http://codereview.chromium.org/501073/diff/8001/39#newcode45 chrome/browser/cocoa/bookmark_groups_controller.mm:45: [self setGroups:groups]; On 2009/12/22 20:02:43, viettrungluu wrote: > Remove 2-spaces of indent. Done. http://codereview.chromium.org/501073/diff/8001/39#newcode53 chrome/browser/cocoa/bookmark_groups_controller.mm:53: [self reload]; On 2009/12/22 20:02:43, viettrungluu wrote: > This doesn't seem so efficient, though I don't know about the implications in > practice. I ran a Shark profile of dragging in a ton of bookmarks, and this method didn't show up at all, so I'm not worried. http://codereview.chromium.org/501073/diff/8001/39#newcode123 chrome/browser/cocoa/bookmark_groups_controller.mm:123: [(ImageAndTextCell*)cell setImage:[manager_ iconForItem:item]]; On 2009/12/22 20:02:43, viettrungluu wrote: > Unindent 2 spaces. *shakes Xcode by the neck until it turns blue* http://codereview.chromium.org/501073/diff/8001/40 File chrome/browser/cocoa/bookmark_manager_controller.h (right): http://codereview.chromium.org/501073/diff/8001/40#newcode23 chrome/browser/cocoa/bookmark_manager_controller.h:23: BookmarkManagerObserver* observer_; On 2009/12/22 20:02:43, viettrungluu wrote: > These appear to be strong. Please use scoped_nsobject unless there's a > compelling reason not to (it'll also get rid of a bunch of code in the > -dealloc). Done. http://codereview.chromium.org/501073/diff/8001/41 File chrome/browser/cocoa/bookmark_manager_controller.mm (right): http://codereview.chromium.org/501073/diff/8001/41#newcode10 chrome/browser/cocoa/bookmark_manager_controller.mm:10: #import "chrome/browser/bookmarks/bookmark_model.h" On 2009/12/22 20:02:43, viettrungluu wrote: > These should be #include? Done. http://codereview.chromium.org/501073/diff/8001/41#newcode36 chrome/browser/cocoa/bookmark_manager_controller.mm:36: public: On 2009/12/22 20:02:43, viettrungluu wrote: > 1-space indent (I don't make these rules ...). Done. http://codereview.chromium.org/501073/diff/8001/41#newcode37 chrome/browser/cocoa/bookmark_manager_controller.mm:37: BookmarkManagerObserver(BookmarkManagerController* manager) On 2009/12/22 20:02:43, viettrungluu wrote: > See the C++ style guide. I'm guessing this should be formatted something like: > > BookmarkManagerObserver(BookmarkManagerController* manager) > : manager_(manager) { } Done. http://codereview.chromium.org/501073/diff/8001/41#newcode41 chrome/browser/cocoa/bookmark_manager_controller.mm:41: virtual void Loaded(BookmarkModel* model) { } On 2009/12/22 20:02:43, viettrungluu wrote: > Could you add a comment saying that it's safe to ignore this since individual > BookmarkNodeAdded() notifications will be sent? Done. http://codereview.chromium.org/501073/diff/8001/41#newcode81 chrome/browser/cocoa/bookmark_manager_controller.mm:81: private: On 2009/12/22 20:02:43, viettrungluu wrote: > 1-space indent. Done. http://codereview.chromium.org/501073/diff/8001/41#newcode93 chrome/browser/cocoa/bookmark_manager_controller.mm:93: NSString* nibPath = [mac_util::MainAppBundle() On 2009/12/22 20:02:43, viettrungluu wrote: > Could you split/align this differently? (This looks as if "pathForResource:" is > aligned with "mac_util:".) Done. http://codereview.chromium.org/501073/diff/8001/41#newcode96 chrome/browser/cocoa/bookmark_manager_controller.mm:96: self = [super initWithWindowNibPath:nibPath owner: self]; On 2009/12/22 20:02:43, viettrungluu wrote: > No space after : (hard to get used to, I know). Done. http://codereview.chromium.org/501073/diff/8001/41#newcode167 chrome/browser/cocoa/bookmark_manager_controller.mm:167: for (int i = node->GetChildCount() -1 ; i >= 0 ; i--) { On 2009/12/22 20:02:43, viettrungluu wrote: > Space after the "-" (binary operator, not unary). Probably no space before the > ";"s, but I'd have to check the style guide to be sure about that. Done. http://codereview.chromium.org/501073/diff/8001/41#newcode221 chrome/browser/cocoa/bookmark_manager_controller.mm:221: On 2009/12/22 20:02:43, viettrungluu wrote: > No blank line here. Done. http://codereview.chromium.org/501073/diff/8001/41#newcode238 chrome/browser/cocoa/bookmark_manager_controller.mm:238: if (!node->is_url()) On 2009/12/22 20:02:43, viettrungluu wrote: > Could you add a DCHECK(node) here? (Possibly also a paranoia-check in the if: > |if (!node || !node->is_url())|.) > > Incidentally, can one (or should one be able to) open a whole folder? Done. http://codereview.chromium.org/501073/diff/8001/43 File chrome/browser/cocoa/bookmark_tree_controller.mm (right): http://codereview.chromium.org/501073/diff/8001/43#newcode93 chrome/browser/cocoa/bookmark_tree_controller.mm:93: if (selectedRows != nil) { On 2009/12/22 20:02:43, viettrungluu wrote: > This one doesn't beep? > > It might be better to check for nil-ness and |return|, and unindent the rest. Good idea. http://codereview.chromium.org/501073/diff/8001/43#newcode166 chrome/browser/cocoa/bookmark_tree_controller.mm:166: if ([ident isEqualToString:@"title"]) { On 2009/12/22 20:02:43, viettrungluu wrote: > I'm not sure how I feel about these hard-coded identifiers, so I'll let it pass. That's what the identifier's for ... it's not exposed in the UI, it's just to recognize columns programmatically. http://codereview.chromium.org/501073/diff/8001/43#newcode171 chrome/browser/cocoa/bookmark_tree_controller.mm:171: return nil; On 2009/12/22 20:02:43, viettrungluu wrote: > Should this ever be reached? Maybe it deserves a NOTREACHED()? Sure. http://codereview.chromium.org/501073/diff/8001/43#newcode185 chrome/browser/cocoa/bookmark_tree_controller.mm:185: GURL url([value UTF8String]); On 2009/12/22 20:02:43, viettrungluu wrote: > Probably you should use base::SysNSStringToUTF8(), or so people have told me. Done. http://codereview.chromium.org/501073/diff/8001/44 File chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm (right): http://codereview.chromium.org/501073/diff/8001/44#newcode12 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:12: On 2009/12/22 23:13:45, viettrungluu wrote: > A general FYI: we sometimes use stuff from Mozilla for URL pasteboard stuff (see > third_party/mozilla/include/NSPasteboard+Utils.*). > > I think it'd be good if we could come up with our own stuff which provides a > superset of the stuff you do and the stuff provided by the Mozilla code. (My > ulterior motive is that the Mozilla code has a large number of deficiencies....) > > This is a "to do" rather than something which you need to address now. OK. http://codereview.chromium.org/501073/diff/8001/44#newcode13 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:13: // Safari uses this type, though it's not declared in any header. On 2009/12/22 23:13:45, viettrungluu wrote: > Are these documented anywhere? Could you either provide documentation or a > pointer to documentation? > > Also, I'm not sure why you use #defines here rather than (static, or in an > anonymous namespace) consts. I found one of them in the WebKit sources and added a note. The other appears to be defined only inside Safari itself. Switched to using consts -- the #defines were just habit. http://codereview.chromium.org/501073/diff/8001/44#newcode19 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:19: // Used internally to indentify intra-outline drags. On 2009/12/22 23:13:45, viettrungluu wrote: > s/indentify/identify > > Also, will it ever be visible to outside applications? (To avoid potential name > conflicts, it might be best to attach "Chrome" or "Chromium" to the the type > name.) Fixed, thanks. And added "Chrome" prefix. http://codereview.chromium.org/501073/diff/8001/44#newcode27 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:27: - (void)_initDragging { On 2009/12/22 23:13:45, viettrungluu wrote: > We don't usually name things beginning with _; maybe just give it a longer, more > descriptive name (without initial _) instead? Done. http://codereview.chromium.org/501073/diff/8001/44#newcode39 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:39: id parentItem = [self itemFromNode:parent]; On 2009/12/22 23:13:45, viettrungluu wrote: > Could you add DCHECK()s to sanity-check childRange.location and > childRange.length? Done. http://codereview.chromium.org/501073/diff/8001/44#newcode71 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:71: [urlStrings addObject:base::SysUTF8ToNSString(node->GetURL().possibly_invalid_spec())]; On 2009/12/22 20:02:43, viettrungluu wrote: > 80 columns. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode91 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:91: // Prepare clipboard and declare types: On 2009/12/22 23:13:45, viettrungluu wrote: > If you just declared a pasteboard and then added types, rather than making an > NSMutableArray and removing types. Yeah, that's cleaner. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode109 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:109: // drags aren't recognized by anyone but us. On 2009/12/22 23:13:45, viettrungluu wrote: > It's not the most likely thing in the world, but will this blow up if you run > two instances of Chromium with different profiles, and drag from one bookmark > manager to the other? No: down in outlineView:acceptDrop: it only uses draggedNodes_ if the source of the drag is the same view. http://codereview.chromium.org/501073/diff/8001/44#newcode165 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:165: static const NSString* kTitleKey = @"Title"; On 2009/12/22 23:13:45, viettrungluu wrote: > Could you provide a description of what's going on? (I'm repeating myself, I > know.) > > It might be cleaner if you wrapped these in an anonymous namespace rather than > using |static|, but it's not a big deal. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode175 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:175: static NSDictionary* makeBookmarkPlistEntry(NSString* name, NSString* urlStr) { On 2009/12/22 23:13:45, viettrungluu wrote: > It's worth mentioning that |name| may be nil, but not |urlStr| (AFAICT). Done. http://codereview.chromium.org/501073/diff/8001/44#newcode176 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:176: if (!name) { On 2009/12/22 23:13:45, viettrungluu wrote: > What if |name != nil| but is an empty string? Is this possible? It could happen; the code here wouldn't mind. It just needs to watch out for nil. http://codereview.chromium.org/501073/diff/8001/44#newcode193 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:193: if( [type isEqualToString:BookmarkDictionaryListPboardType] ) { On 2009/12/22 23:13:45, viettrungluu wrote: > Our style guide dictates (for better or worse): > > if ([type ...]) { > > (space after |if|, no spaces inside parens). Oops. Not sure if that was me, or the Apple sample that I swiped part of this code from... http://codereview.chromium.org/501073/diff/8001/44#newcode197 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:197: } else if( [type isEqualToString:WebURLsWithTitlesPboardType] ) { On 2009/12/22 23:13:45, viettrungluu wrote: > Here too. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode222 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:222: return [NSArray arrayWithObject:makeBookmarkPlistEntry(urlStr, title)]; On 2009/12/22 23:13:45, viettrungluu wrote: > urlStr <-> title. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode235 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:235: it < nodes.end(); it++) { On 2009/12/22 23:13:45, viettrungluu wrote: > ++it, not it++. (Probably |it != nodes.end()| would be more idiomatic than <, > but I'm too lazy to figure out if it makes a difference. Probably not in this > case.) Done. http://codereview.chromium.org/501073/diff/8001/44#newcode236 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:236: NSAutoreleasePool* pool = [NSAutoreleasePool new]; On 2009/12/22 23:13:45, viettrungluu wrote: > I don't object, but is there a reason you need/want to do this? Probably worth a > comment. Some of the observers (esp. BookmarkMenuBridge) allocate a lot of autoreleased objects every time a bookmark changes. That's the reason for the memory bloat reported in bug 29802. This loop can move a lot of bookmarks, so clean up after each one. Added a comment. http://codereview.chromium.org/501073/diff/8001/44#newcode256 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:256: int i = 0; On 2009/12/22 23:13:45, viettrungluu wrote: > Probably if |dstIndex| is an NSInteger, so too should |i|. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode265 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:265: dstIndex + i++, On 2009/12/22 23:13:45, viettrungluu wrote: > I think I'm legally obligated to object to |dstIndex + i++|; better to move the > i++ outside the function call. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode275 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:275: dstIndex + i++, On 2009/12/22 23:13:45, viettrungluu wrote: > Ditto. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode359 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:359: if ([info draggingSource] == outlineView && On 2009/12/22 23:13:45, viettrungluu wrote: > Okay, I guess it won't blow up. See, I told you so :) http://codereview.chromium.org/501073/diff/8001/44#newcode380 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:380: NSArray* plist = [self readPropertyListFromPasteboard:[NSPasteboard generalPasteboard]]; On 2009/12/22 23:13:45, viettrungluu wrote: > 80 columns. Done. http://codereview.chromium.org/501073/diff/8001/44#newcode382 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:382: NSBeep(); On 2009/12/22 23:13:45, viettrungluu wrote: > Given what you do in |-validateMenuItem:|, do you expect the NSBeep()s (here and > elsewhere) to be called? If not, it might be worth a NOTREACHED() or a > LOG(WARNING) << "...", or whatever. -validateMenuItem: only checks the pasteboard types, not their contents, so it's possible for the contents to be unreadable, and we need to catch this. I like to put in a beep so there's some user feedback that it didn't work (since this is a UI action method.) http://codereview.chromium.org/501073/diff/8001/45 File chrome/browser/cocoa/browser_window_cocoa.mm (right): http://codereview.chromium.org/501073/diff/8001/45#newcode257 chrome/browser/cocoa/browser_window_cocoa.mm:257: [BookmarkManagerController showBookmarkManager: browser_->profile()]; On 2009/12/22 20:02:43, viettrungluu wrote: > No space after :. Done. http://codereview.chromium.org/501073/diff/8001/50 File third_party/apple/README.chromium (right): http://codereview.chromium.org/501073/diff/8001/50#newcode1 third_party/apple/README.chromium:1: This is Chrome's local copy of Apple sample code, On 2009/12/22 20:02:43, viettrungluu wrote: > Probably this should be "Chromium" (given that this is README.chromium). Done. http://codereview.chromium.org/501073/diff/8001/50#newcode6 third_party/apple/README.chromium:6: * ImageAndTextCell.h: Changed 'image' property to 'retain' mode to fix a crash. On 2009/12/22 20:02:43, viettrungluu wrote: > 80 columns? Done.
Some quick comments from SFO.... http://codereview.chromium.org/501073/diff/13001/14007 File chrome/browser/cocoa/bookmark_groups_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14007#newcode17 chrome/browser/cocoa/bookmark_groups_controller_unittest.mm:17: I don't know if our style guide outlaws a blank line here, but I don't think I've ever seen one after public: (et al.) in our code. http://codereview.chromium.org/501073/diff/13001/14009 File chrome/browser/cocoa/bookmark_manager_controller.mm (right): http://codereview.chromium.org/501073/diff/13001/14009#newcode34 chrome/browser/cocoa/bookmark_manager_controller.mm:34: class BookmarkManagerObserver : public BookmarkModelObserver { It occurs to me that the name of this class is confusing -- it kind of implies that this class observes the bookmark manager (just as a |BookmarkModelObserver| observes the model). I think we've been naming such classes "Bridge" -- so probably it should be called |BookmarkManagerBridge|, which parallels |BookmarkBarBridge|. http://codereview.chromium.org/501073/diff/13001/14010 File chrome/browser/cocoa/bookmark_manager_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14010#newcode34 chrome/browser/cocoa/bookmark_manager_controller_unittest.mm:34: NSWindow *w = [controller_ window]; NSWindow* http://codereview.chromium.org/501073/diff/13001/14010#newcode36 chrome/browser/cocoa/bookmark_manager_controller_unittest.mm:36: ASSERT_TRUE([w isVisible]); This can/should probably be EXPECT_TRUE(); in general, unless the test would blow up (i.e., crash), EXPECT_*() is preferred. http://codereview.chromium.org/501073/diff/13001/14010#newcode55 chrome/browser/cocoa/bookmark_manager_controller_unittest.mm:55: ASSERT_NE(bar, other); What I said above.... http://codereview.chromium.org/501073/diff/13001/14014 File chrome/browser/cocoa/bookmark_tree_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14014#newcode18 chrome/browser/cocoa/bookmark_tree_controller_unittest.mm:18: Probably no blank line here. http://codereview.chromium.org/501073/diff/13001/14016 File chrome/browser/cocoa/nsmenuitem_additions_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14016#newcode1 chrome/browser/cocoa/nsmenuitem_additions_unittest.mm:1: #import "chrome/browser/cocoa/nsmenuitem_additions.h" Did you mean to include changes to this file in your CL? (I'm guessing not, but if you did, I'd prefer a separate CL for this.)
I wish I could hit "reply" *and* have it send all my other comments.... On 2009/12/23 17:02:45, Jens Alfke wrote: > On 2009/12/22 20:02:43, viettrungluu wrote: > > Unindent 2 spaces. > > *shakes Xcode by the neck until it turns blue* LOL. Being a minimalist vim user (in that I never set up anything remotely fancy), I'm always to blame for all my faulty indentation. > http://codereview.chromium.org/501073/diff/8001/43#newcode166 > chrome/browser/cocoa/bookmark_tree_controller.mm:166: if ([ident > isEqualToString:@"title"]) { > On 2009/12/22 20:02:43, viettrungluu wrote: > > I'm not sure how I feel about these hard-coded identifiers, so I'll let it > pass. > > That's what the identifier's for ... it's not exposed in the UI, it's just to > recognize columns programmatically. I just meant directly writing @"identifier" rather than putting it in a const. |kHardToReadIdentifier| loses on readability, but wins slightly on the compiler providing some consistency checks. (Of course, it won't protect you from mixing it up with |kDifferentHardToReadIdentifier|, which happens easily with autocompletion.) Hence my ambivalence about all the options. :-|
I've fixed the noted issues, and I'm about to upload a patch that also adds copy and paste unit tests. http://codereview.chromium.org/501073/diff/13001/14007 File chrome/browser/cocoa/bookmark_groups_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14007#newcode17 chrome/browser/cocoa/bookmark_groups_controller_unittest.mm:17: On 2009/12/24 05:04:23, viettrungluu wrote: > I don't know if our style guide outlaws a blank line here, but I don't think > I've ever seen one after public: (et al.) in our code. OK, whatevs. http://codereview.chromium.org/501073/diff/13001/14009 File chrome/browser/cocoa/bookmark_manager_controller.mm (right): http://codereview.chromium.org/501073/diff/13001/14009#newcode34 chrome/browser/cocoa/bookmark_manager_controller.mm:34: class BookmarkManagerObserver : public BookmarkModelObserver { On 2009/12/24 05:04:23, viettrungluu wrote: > It occurs to me that the name of this class is confusing -- it kind of implies > that this class observes the bookmark manager (just as a |BookmarkModelObserver| > observes the model). I think we've been naming such classes "Bridge" -- so > probably it should be called |BookmarkManagerBridge|, which parallels > |BookmarkBarBridge|. Done. http://codereview.chromium.org/501073/diff/13001/14010 File chrome/browser/cocoa/bookmark_manager_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14010#newcode34 chrome/browser/cocoa/bookmark_manager_controller_unittest.mm:34: NSWindow *w = [controller_ window]; On 2009/12/24 05:04:23, viettrungluu wrote: > NSWindow* Done. http://codereview.chromium.org/501073/diff/13001/14010#newcode36 chrome/browser/cocoa/bookmark_manager_controller_unittest.mm:36: ASSERT_TRUE([w isVisible]); On 2009/12/24 05:04:23, viettrungluu wrote: > This can/should probably be EXPECT_TRUE(); in general, unless the test would > blow up (i.e., crash), EXPECT_*() is preferred. Done. http://codereview.chromium.org/501073/diff/13001/14010#newcode55 chrome/browser/cocoa/bookmark_manager_controller_unittest.mm:55: ASSERT_NE(bar, other); On 2009/12/24 05:04:23, viettrungluu wrote: > What I said above.... Done. http://codereview.chromium.org/501073/diff/13001/14014 File chrome/browser/cocoa/bookmark_tree_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/13001/14014#newcode18 chrome/browser/cocoa/bookmark_tree_controller_unittest.mm:18: On 2009/12/24 05:04:23, viettrungluu wrote: > Probably no blank line here. Done.
Thanks. LGTM with: - tiny nits (below) fixed, - description of xib file changes put into CL description, and - clean run on the trybots. Also watch that the unit tests run cleanly under valgrind. (Warning: IIRC, we've had problems with pasteboards in unit tests. If you have trouble with valgrind, and you're sure it's not your fault, you may need to add a suppression or exclusion.) http://codereview.chromium.org/501073/diff/16008/16021 File chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm (right): http://codereview.chromium.org/501073/diff/16008/16021#newcode155 chrome/browser/cocoa/bookmark_tree_controller_pasteboard.mm:155: return [self writeItems:[self selectedItems] Unindent 2 spaces. http://codereview.chromium.org/501073/diff/16008/16022 File chrome/browser/cocoa/bookmark_tree_controller_unittest.mm (right): http://codereview.chromium.org/501073/diff/16008/16022#newcode16 chrome/browser/cocoa/bookmark_tree_controller_unittest.mm:16: @"WebURLsWithTitlesPboardType"; Indent (second line) 4 spaces.
Just so this doesn't get lost: > Tnanks. Added a README.chromium; the sample code already has its own license > inline so I don't think a separate file is necessary. It is necessary, see http://wiki.corp.google.com/twiki/bin/view/Main/ThirdParty#Document_the_Code_... . Looks like we have no version of this on the chromium wiki :-( You can copy the license from the source file and put this into a new file called "license".
On 2010/01/05 00:20:41, Nico wrote: > It is necessary, see > http://wiki.corp.google.com/twiki/bin/view/Main/ThirdParty#Document_the_Code_... ImageAndTextCell is actually being checked in as a separate patch now, because it's necessary for another feature. It's being reviewed at <http://codereview.chromium.org/523045> ... looks like the license file is already there.
On 2010/01/05 00:31:51, Jens Alfke wrote: > On 2010/01/05 00:20:41, Nico wrote: > > It is necessary, see > > > http://wiki.corp.google.com/twiki/bin/view/Main/ThirdParty#Document_the_Code_... > > ImageAndTextCell is actually being checked in as a separate patch now, because > it's necessary for another feature. It's being reviewed at > <http://codereview.chromium.org/523045> ... looks like the license file is > already there. Oh, cool. Props to the reviewer of that patch ;-)
LGTM Glad to see this come together. http://codereview.chromium.org/501073/diff/16008/16029 File third_party/apple/README.chromium (right): http://codereview.chromium.org/501073/diff/16008/16029#newcode6 third_party/apple/README.chromium:6: * ImageAndTextCell.h: Changed 'image' property to 'retain' mode to fix a crash. ha. Be sure to upstream this. :-) |
