Implement Bookmark All Tabs... using the BookmarkEditorController and determi...

Implement Bookmark All Tabs... Added an abstraction of BookmarkEditorController called BookmarkEditorBaseController, from which BookmarkEditorController and the new BookmarkAllTabsController derive.  The bookmark/folder name, URL and OK button now use bindings/KVO.  Added BookmarkAllTabs.xib which is nearly identical to BookmarkEditor.xib.  Changed unit test since an empty bookmark name is now acceptable.  Added unit test for the Bookmark All Tabs...

BookmarkEditor.xib changes: Removed the bookmark name and url as outlets and instead bound their values to controller's displayName and displayURL respectively.  Bound the OK button enabling to the controller's okEnabled.

BUG=25099
TEST=Open two or more tabs.  Bring up a contextual menu by control-clicking on one of the tabs.  Choose the Bookmark All Tabs... menu item.  A dialog will be presented in which you can type a folder name and select the parent folder in which the new folder will be placed.  By default, the most recently touched folder will be selected as the parent.  Click OK.  Now check to parent folder to see that the newly created folder has been added and then check within that folder to see that the open tabs have been added.

Perform the above operation again only this time choose the Bookmark All Tabs... menu item found in the Bookmarks menu.


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

[pink (ping after 24hrs), 2009-11-04 14:24:16]

Was there a reason why you needed to create nearly identical nibs? Was there
nothing you could do in awakeFromNib to re-use them? Maybe add a comment in the
code as to why, and include how they differ?

http://codereview.chromium.org/357005/diff/2001/3005
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/357005/diff/2001/3005#newcode114
Line 114: NSSet* keys = [NSSet setWithObjects:@"displayName", @"displayURL",
nil];
why not just return these in a single line rather than breaking it up into decl
+ return? It's not like you're really going to need to debug this method, it's
pretty simple.

http://codereview.chromium.org/357005/diff/2001/3005#newcode126
Line 126: (bookmarkAllTabs) ? @"BookmarkAllTabs" : @"BookmarkEditor";
no need for parens

http://codereview.chromium.org/357005/diff/2001/3005#newcode160
Line 160: l10n_util::GetString(IDS_BOOMARK_EDITOR_NEW_FOLDER_NAME);
continuation lines indent an additional 4 spaces.

however, you should probably use GetNSStringWithFixup() as it does what you
need.

http://codereview.chromium.org/357005/diff/2001/3005#newcode340
Line 340: // non-blank title.
you probalby mean "non-empty", since blank titles are technically allowed.

http://codereview.chromium.org/357005/diff/2001/3005#newcode344
Line 344: okEnabled = ([[[self displayName] stringByTrimmingCharactersInSet:
doesn't this get us back to the case where you can't have a whitespace-only name
for a title? Didn't we already decide this was ok? Or am I confused?

http://codereview.chromium.org/357005/diff/2001/3005#newcode376
Line 376: Browser* browser = BrowserList::GetLastActive();
this dialog is window-modal, so what happens if while the user is filling it
out, the user makes a different window frontmost? Won't it grab the Browser for
the wrong window?

http://codereview.chromium.org/357005/diff/2001/3005#newcode377
Line 377: TabStripModel* tabstrip_model = browser->tabstrip_model();
It makes me sad that the bookmark code has to know about the tab model. Is there
no way to abstract this?

[pink (ping after 24hrs), 2009-11-04 14:26:39]

Sigh, it submitted before I was ready.

http://codereview.chromium.org/357005/diff/2001/3005
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/357005/diff/2001/3005#newcode376
Line 376: Browser* browser = BrowserList::GetLastActive();
On 2009/11/04 14:24:16, pink wrote:
> this dialog is window-modal, so what happens if while the user is filling it
> out, the user makes a different window frontmost? Won't it grab the Browser
for
> the wrong window?

Never mind, clearly this is wrong (since the window has to be frontmost to OK
the dialog), but I can't remove this comment.

[John Grabowski, 2009-11-04 18:22:10]

http://codereview.chromium.org/357005/diff/2001/3006
File chrome/browser/cocoa/bookmark_editor_controller.h (right):

http://codereview.chromium.org/357005/diff/2001/3006#newcode43
Line 43: IBOutlet NSButton* okButton_;
place with other IBOutlets

http://codereview.chromium.org/357005/diff/2001/3005
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/357005/diff/2001/3005#newcode118
Line 118: - (id)initWithParentWindow:(NSWindow*)parentWindow
Overall comment for entire file: you've got a number of
if (bookmarkAlLTabs) {  }
conditionals in a number of functions.  It's not messy per se but is headed in
that direction.  Wouldn't subclassing make this simpler?

http://codereview.chromium.org/357005/diff/2001/3005#newcode131
Line 131: [self setBookmarkAllTabs:bookmarkAllTabs];
Sure, it's the same as bookmarkAllTabs_ = bookmarkAllTabs, but it doesn't match
other calls right here so seems like there is more hidden.

http://codereview.chromium.org/357005/diff/2001/3005#newcode160
Line 160: l10n_util::GetString(IDS_BOOMARK_EDITOR_NEW_FOLDER_NAME);
On 2009/11/04 14:24:16, pink wrote:
> continuation lines indent an additional 4 spaces.
> 
> however, you should probably use GetNSStringWithFixup() as it does what you
> need.

ditto pink's comment

http://codereview.chromium.org/357005/diff/2001/3005#newcode381
Line 381: const std::wstring tabTitle = UTF16ToWideHack(tc->GetTitle());
Abstract this loop into a method called TabContentThingies: which returns a
std::vector of std::pair<Title,URL>.  See next file for more info on why.

http://codereview.chromium.org/357005/diff/2001/3004
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/357005/diff/2001/3004#newcode365
Line 365: #if 0
A "TODO" is not OK here.
Either fix 10899 or abstract out calls to TabContents (see TabContentThingies:
comment in other file) so that you can override it in your unit test and make
everything else work fine.

[mrossetti, 2009-11-05 18:14:55]

All issues addressed.  Added an abstraction of the BookmarkEditorController and
a new controller based on that abstraction for doing the BookmarkAllTabs work.

Thanks for the suggestions!

http://codereview.chromium.org/357005/diff/2001/3006
File chrome/browser/cocoa/bookmark_editor_controller.h (right):

http://codereview.chromium.org/357005/diff/2001/3006#newcode43
Line 43: IBOutlet NSButton* okButton_;
On 2009/11/04 18:22:10, John Grabowski wrote:
> place with other IBOutlets

Done.

http://codereview.chromium.org/357005/diff/2001/3005
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/357005/diff/2001/3005#newcode114
Line 114: NSSet* keys = [NSSet setWithObjects:@"displayName", @"displayURL",
nil];
On 2009/11/04 14:24:16, pink wrote:
> why not just return these in a single line rather than breaking it up into
decl
> + return? It's not like you're really going to need to debug this method, it's
> pretty simple.

Done.

http://codereview.chromium.org/357005/diff/2001/3005#newcode118
Line 118: - (id)initWithParentWindow:(NSWindow*)parentWindow
On 2009/11/04 18:22:10, John Grabowski wrote:
> Overall comment for entire file: you've got a number of
> if (bookmarkAlLTabs) {  }
> conditionals in a number of functions.  It's not messy per se but is headed in
> that direction.  Wouldn't subclassing make this simpler? 

Done.

http://codereview.chromium.org/357005/diff/2001/3005#newcode126
Line 126: (bookmarkAllTabs) ? @"BookmarkAllTabs" : @"BookmarkEditor";
On 2009/11/04 14:24:16, pink wrote:
> no need for parens

This code has been eliminated.

http://codereview.chromium.org/357005/diff/2001/3005#newcode131
Line 131: [self setBookmarkAllTabs:bookmarkAllTabs];
On 2009/11/04 18:22:10, John Grabowski wrote:
> Sure, it's the same as bookmarkAllTabs_ = bookmarkAllTabs, but it doesn't
match
> other calls right here so seems like there is more hidden.

This code is now gone.

http://codereview.chromium.org/357005/diff/2001/3005#newcode160
Line 160: l10n_util::GetString(IDS_BOOMARK_EDITOR_NEW_FOLDER_NAME);
On 2009/11/04 14:24:16, pink wrote:
> continuation lines indent an additional 4 spaces.
> 
> however, you should probably use GetNSStringWithFixup() as it does what you
> need.

Done.

http://codereview.chromium.org/357005/diff/2001/3005#newcode160
Line 160: l10n_util::GetString(IDS_BOOMARK_EDITOR_NEW_FOLDER_NAME);
On 2009/11/04 14:24:16, pink wrote:
> continuation lines indent an additional 4 spaces.
> 
> however, you should probably use GetNSStringWithFixup() as it does what you
> need.

Done.

http://codereview.chromium.org/357005/diff/2001/3005#newcode340
Line 340: // non-blank title.
On 2009/11/04 14:24:16, pink wrote:
> you probalby mean "non-empty", since blank titles are technically allowed.

Done.

http://codereview.chromium.org/357005/diff/2001/3005#newcode344
Line 344: okEnabled = ([[[self displayName] stringByTrimmingCharactersInSet:
On 2009/11/04 14:24:16, pink wrote:
> doesn't this get us back to the case where you can't have a whitespace-only
name
> for a title? Didn't we already decide this was ok? Or am I confused?

Can folders have blank/empty names?  I'll have to check.

http://codereview.chromium.org/357005/diff/2001/3005#newcode381
Line 381: const std::wstring tabTitle = UTF16ToWideHack(tc->GetTitle());
On 2009/11/04 18:22:10, John Grabowski wrote:
> Abstract this loop into a method called TabContentThingies: which returns a
> std::vector of std::pair<Title,URL>.  See next file for more info on why.

Done.

http://codereview.chromium.org/357005/diff/2001/3004
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/357005/diff/2001/3004#newcode365
Line 365: #if 0
On 2009/11/04 18:22:10, John Grabowski wrote:
> A "TODO" is not OK here.
> Either fix 10899 or abstract out calls to TabContents (see TabContentThingies:
> comment in other file) so that you can override it in your unit test and make
> everything else work fine.

Done.

[mrossetti, 2009-11-05 20:04:22]

Uploaded missing files.

[John Grabowski, 2009-11-05 21:11:03]

Almost there.
Please add some more simple unit tests, merge in dmac's CL, and I'll be happy.

http://codereview.chromium.org/357005/diff/4008/8009
File chrome/browser/cocoa/bookmark_all_tabs_controller.h (right):

http://codereview.chromium.org/357005/diff/4008/8009#newcode13
Line 13: typedef std::vector<ActiveTabNameURLPair> ActiveTabsNameURLPairList;
type is vector but name is BlahBlahList?
How about BlahBlahArray?

http://codereview.chromium.org/357005/diff/4008/8007
File chrome/browser/cocoa/bookmark_all_tabs_controller.mm (right):

http://codereview.chromium.org/357005/diff/4008/8007#newcode36
Line 36: [super awakeFromNib];
Perhaps [super awakeFromNib] first?  Especially before calling the base class's
setInitialName:.

http://codereview.chromium.org/357005/diff/4008/8007#newcode59
Line 59: NSString* name = [[self displayName] stringByTrimmingCharactersInSet:
What if I want to name the folder "       "?

http://codereview.chromium.org/357005/diff/4008/8008
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/357005/diff/4008/8008#newcode21
Line 21: // NOTE: Derived classes must be dispatched via the
|BookmarkEditor::Show|
Not sure about "must be".
Also, use pipes for args, not functions, in comments.

http://codereview.chromium.org/357005/diff/4008/8008#newcode63
Line 63: // The OK action will record the edited bookmark.  The default
dismisses
space above this line

http://codereview.chromium.org/357005/diff/4008/8010
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/357005/diff/4008/8010#newcode73
Line 73: const BookmarkNode* GetFolderChildForParent(const BookmarkNode*
parent_node,
unit test this dude

http://codereview.chromium.org/357005/diff/4008/8010#newcode89
Line 89: int IndexOfFolderChild(const BookmarkNode* child_node) {
unit test this dude

http://codereview.chromium.org/357005/diff/4008/8010#newcode181
Line 181: - (void)selectNodeInBrowser:(const BookmarkNode*)node {
unit test which calls selectNodeInBrowser: then confirms value with selectedNode
below.

http://codereview.chromium.org/357005/diff/4008/8010#newcode300
Line 300: - (BOOL)okEnabled {
No longer needed?

http://codereview.chromium.org/357005/diff/4008/8010#newcode326
Line 326: [self autorelease];
Be sure to merge in dmac's CL at http://codereview.chromium.org/361011 which
impacts this function specifically.  Recommend you do that before trying to land
given file reorg.

http://codereview.chromium.org/357005/diff/4008/8010#newcode339
Line 339: - (BOOL)okButtonEnabled {
Obsolete?

http://codereview.chromium.org/357005/diff/4008/8005
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/357005/diff/4008/8005#newcode60
Line 60: [super awakeFromNib];
Possibly call [super awakeFromNib] first?

http://codereview.chromium.org/357005/diff/4008/8004
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/357005/diff/4008/8004#newcode134
Line 134: EXPECT_TRUE([default_controller_ okButtonEnabled]);
OK buttons always enabled so no more need for unit test.

[mrossetti, 2009-11-06 01:58:57]

Merged with latest from dmaclach.

http://codereview.chromium.org/357005/diff/4008/8009
File chrome/browser/cocoa/bookmark_all_tabs_controller.h (right):

http://codereview.chromium.org/357005/diff/4008/8009#newcode13
Line 13: typedef std::vector<ActiveTabNameURLPair> ActiveTabsNameURLPairList;
On 2009/11/05 21:11:03, John Grabowski wrote:
> type is vector but name is BlahBlahList?
> How about BlahBlahArray?

Done.

http://codereview.chromium.org/357005/diff/4008/8007
File chrome/browser/cocoa/bookmark_all_tabs_controller.mm (right):

http://codereview.chromium.org/357005/diff/4008/8007#newcode36
Line 36: [super awakeFromNib];
On 2009/11/05 21:11:03, John Grabowski wrote:
> Perhaps [super awakeFromNib] first?  Especially before calling the base
class's
> setInitialName:.
> 

It's gotta be last since the super's uses the initialName.

http://codereview.chromium.org/357005/diff/4008/8007#newcode59
Line 59: NSString* name = [[self displayName] stringByTrimmingCharactersInSet:
On 2009/11/05 21:11:03, John Grabowski wrote:
> What if I want to name the folder "       "?
> 

Yeah, I should only be stripping out newlines.

http://codereview.chromium.org/357005/diff/4008/8008
File chrome/browser/cocoa/bookmark_editor_base_controller.h (right):

http://codereview.chromium.org/357005/diff/4008/8008#newcode21
Line 21: // NOTE: Derived classes must be dispatched via the
|BookmarkEditor::Show|
On 2009/11/05 21:11:03, John Grabowski wrote:
> Not sure about "must be".
> Also, use pipes for args, not functions, in comments.
> 

Done.

http://codereview.chromium.org/357005/diff/4008/8008#newcode63
Line 63: // The OK action will record the edited bookmark.  The default
dismisses
On 2009/11/05 21:11:03, John Grabowski wrote:
> space above this line
> 

Done.

http://codereview.chromium.org/357005/diff/4008/8010
File chrome/browser/cocoa/bookmark_editor_base_controller.mm (right):

http://codereview.chromium.org/357005/diff/4008/8010#newcode73
Line 73: const BookmarkNode* GetFolderChildForParent(const BookmarkNode*
parent_node,
On 2009/11/05 21:11:03, John Grabowski wrote:
> unit test this dude

Done.

http://codereview.chromium.org/357005/diff/4008/8010#newcode89
Line 89: int IndexOfFolderChild(const BookmarkNode* child_node) {
On 2009/11/05 21:11:03, John Grabowski wrote:
> unit test this dude

Done.

http://codereview.chromium.org/357005/diff/4008/8010#newcode181
Line 181: - (void)selectNodeInBrowser:(const BookmarkNode*)node {
On 2009/11/05 21:11:03, John Grabowski wrote:
> unit test which calls selectNodeInBrowser: then confirms value with
selectedNode
> below.

Done.

http://codereview.chromium.org/357005/diff/4008/8010#newcode300
Line 300: - (BOOL)okEnabled {
On 2009/11/05 21:11:03, John Grabowski wrote:
> No longer needed?

It's needed.  The BookmarkAllTabsController doesn't provide an -[okEnabled]
since it's always enabled.  I _could_ default to NO here and override in
BookmarkAllTabsController, but didn't see a need for that.

http://codereview.chromium.org/357005/diff/4008/8010#newcode326
Line 326: [self autorelease];
On 2009/11/05 21:11:03, John Grabowski wrote:
> Be sure to merge in dmac's CL at http://codereview.chromium.org/361011 which
> impacts this function specifically.  Recommend you do that before trying to
land
> given file reorg.

Done.

http://codereview.chromium.org/357005/diff/4008/8010#newcode339
Line 339: - (BOOL)okButtonEnabled {
On 2009/11/05 21:11:03, John Grabowski wrote:
> Obsolete?

No, I'm adding a unit test to BookmarkAllTabs to insure that the OK button is
always enabled.

http://codereview.chromium.org/357005/diff/4008/8005
File chrome/browser/cocoa/bookmark_editor_controller.mm (right):

http://codereview.chromium.org/357005/diff/4008/8005#newcode60
Line 60: [super awakeFromNib];
On 2009/11/05 21:11:03, John Grabowski wrote:
> Possibly call [super awakeFromNib] first?

I'd like to keep it consistent with the same call made from
BookmarkAllTabsController.  In this case it doesn't matter, but in BATC it makes
a difference.

http://codereview.chromium.org/357005/diff/4008/8004
File chrome/browser/cocoa/bookmark_editor_controller_unittest.mm (right):

http://codereview.chromium.org/357005/diff/4008/8004#newcode134
Line 134: EXPECT_TRUE([default_controller_ okButtonEnabled]);
On 2009/11/05 21:11:03, John Grabowski wrote:
> OK buttons always enabled so no more need for unit test.

The OK button is only enabled when there is a valid URL.  So we still need this
test.

[John Grabowski, 2009-11-06 02:06:19]

LGTM