From f8a0cb584078998db374e3df2e0f4290377b0be6 Mon Sep 17 00:00:00 2001 From: "pinkerton@chromium.org" Date: Wed, 16 Dec 2009 16:21:36 +0000 Subject: Factor tab context menu into a shared model and fix mac and win to use it. Improve a couple of model unit tests. Remove unused members in the models. BUG=28977 TEST=context menus on tabs should work and enable/disable properly Review URL: http://codereview.chromium.org/500030 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@34718 0039d316-1c4b-4281-b951-d872f2087c98 --- chrome/app/nibs/TabView.xib | 1120 +---------------------- chrome/browser/app_menu_model.h | 2 - chrome/browser/app_menu_model_unittest.cc | 36 +- chrome/browser/cocoa/menu_controller.mm | 3 +- chrome/browser/cocoa/tab_controller.h | 12 +- chrome/browser/cocoa/tab_controller.mm | 89 +- chrome/browser/cocoa/tab_controller_unittest.mm | 13 + chrome/browser/cocoa/tab_view.mm | 6 + chrome/browser/cocoa/tab_view_unittest.mm | 5 + chrome/browser/page_menu_model.h | 7 +- chrome/browser/tab_menu_model.cc | 43 + chrome/browser/tab_menu_model.h | 25 + chrome/browser/tab_menu_model_unittest.cc | 68 ++ chrome/browser/views/tabs/tab.cc | 29 +- chrome/chrome_browser.gypi | 2 + chrome/chrome_tests.gypi | 1 + 16 files changed, 300 insertions(+), 1161 deletions(-) create mode 100644 chrome/browser/tab_menu_model.cc create mode 100644 chrome/browser/tab_menu_model.h create mode 100644 chrome/browser/tab_menu_model_unittest.cc diff --git a/chrome/app/nibs/TabView.xib b/chrome/app/nibs/TabView.xib index 26b175e..1daed60 100644 --- a/chrome/app/nibs/TabView.xib +++ b/chrome/app/nibs/TabView.xib @@ -1,19 +1,14 @@ - + 1050 - 10C540 - 740 - 1038.25 - 458.00 - - com.apple.InterfaceBuilder.CocoaPlugin - 740 - + 9L31a + 677 + 949.54 + 353.00 YES - YES @@ -21,7 +16,7 @@ YES - + YES @@ -49,7 +44,7 @@ 256 YES - + YES Apple PDF pasteboard type Apple PICT pasteboard type @@ -81,7 +76,7 @@ 300 YES - + YES Apple PDF pasteboard type Apple PICT pasteboard type @@ -120,7 +115,7 @@ LucidaGrande - 13 + 1.300000e+01 1044 @@ -152,7 +147,7 @@ Label LucidaGrande - 11 + 1.100000e+01 3100 @@ -162,7 +157,7 @@ controlColor 3 - MC42NjY2NjY2NjY3AA + MC42NjY2NjY2OQA @@ -181,138 +176,6 @@ TabView - - Context Menu - - YES - - - ^IDS_TAB_CXMENU_NEWTAB - - 2147483647 - - NSImage - NSMenuCheckmark - - - NSImage - NSMenuMixedState - - 1 - - - - YES - YES - - - 2147483647 - - - - - - ^IDS_TAB_CXMENU_RELOAD - - 2147483647 - - - 2 - - - - ^IDS_TAB_CXMENU_DUPLICATE - - 2147483647 - - - 3 - - - - ^IDS_TAB_CXMENU_PIN_TAB_MAC - - 2147483647 - - - 9 - - - - YES - YES - - - 2147483647 - - - - - - ^IDS_TAB_CXMENU_CLOSETAB - - 2147483647 - - - 4 - - - - ^IDS_TAB_CXMENU_CLOSEOTHERTABS - - 2147483647 - - - 5 - - - - ^IDS_TAB_CXMENU_CLOSETABSTORIGHT - - 2147483647 - - - 6 - - - - ^IDS_TAB_CXMENU_CLOSETABSOPENEDBY - - 2147483647 - - - 7 - - - - YES - YES - - - 2147483647 - - - - - - ^IDS_RESTORE_TAB - - 2147483647 - - - 8 - - - - ^IDS_TAB_CXMENU_BOOKMARK_ALL_TABS - - 2147483647 - - - 10 - - - ChromeUILocalizer @@ -362,86 +225,6 @@ - contextMenu_ - - - - 63 - - - - menu - - - - 65 - - - - commandDispatch: - - - - 81 - - - - commandDispatch: - - - - 82 - - - - commandDispatch: - - - - 83 - - - - commandDispatch: - - - - 84 - - - - commandDispatch: - - - - 85 - - - - commandDispatch: - - - - 86 - - - - commandDispatch: - - - - 87 - - - - commandDispatch: - - - - 88 - - - closeButton_ @@ -496,56 +279,34 @@ 98 - - - commandDispatch: - - - - 101 - - - - commandDispatch: - - - - 103 - - - - delegate - - - - 104 - YES 0 - + + YES + -2 - - File's Owner + + RmlsZSdzIE93bmVyA -1 - + First Responder -3 - + Application @@ -558,7 +319,7 @@ - + 8 @@ -603,80 +364,9 @@ - 59 - - - YES - - - - - - - - - - - - - - - - - - 60 - - - - - 61 - - - - - 62 - - - - - 66 - - - - - 67 - - - - - 68 - - - - - 69 - - - - - 70 - - - - - 71 - - - - - 72 - - - - 90 - + 94 @@ -692,26 +382,11 @@ - - 99 - - - - - 100 - - - - - 102 - - - YES - + YES -3.IBPluginDependency 1.IBEditorWindowLastContentRect @@ -719,31 +394,16 @@ 1.IBViewEditorWindowController.showingLayoutRectangles 1.WindowOrigin 1.editorWindowContentRectSynchronizationRect - 100.IBPluginDependency - 102.IBPluginDependency 50.CustomClassName 50.IBPluginDependency 51.IBPluginDependency 55.IBPluginDependency 56.IBPluginDependency - 59.IBEditorWindowLastContentRect - 59.IBPluginDependency - 60.IBPluginDependency - 61.IBPluginDependency - 62.IBPluginDependency - 66.IBPluginDependency - 67.IBPluginDependency - 68.IBPluginDependency - 69.IBPluginDependency - 70.IBPluginDependency - 71.IBPluginDependency - 72.IBPluginDependency 8.IBPluginDependency 9.IBPluginDependency 94.IBPluginDependency 95.CustomClassName 95.IBPluginDependency - 99.IBPluginDependency YES @@ -753,36 +413,23 @@ {628, 654} {{217, 442}, {480, 272}} - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin HoverCloseButton com.apple.InterfaceBuilder.CocoaPlugin com.apple.InterfaceBuilder.CocoaPlugin com.apple.InterfaceBuilder.CocoaPlugin com.apple.InterfaceBuilder.CocoaPlugin - {{358, 508}, {340, 233}} - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin com.apple.InterfaceBuilder.CocoaPlugin com.apple.InterfaceBuilder.CocoaPlugin com.apple.InterfaceBuilder.CocoaPlugin GTMFadeTruncatingTextFieldCell com.apple.InterfaceBuilder.CocoaPlugin - com.apple.InterfaceBuilder.CocoaPlugin YES - + + YES + YES @@ -790,7 +437,9 @@ YES - + + YES + YES @@ -838,7 +487,7 @@ NSObject YES - + YES otherObjectToLocalize_ owner_ @@ -865,13 +514,6 @@ - NSMenuItem - - IBProjectSource - browser/cocoa/nsmenuitem_additions.h - - - NSObject IBProjectSource @@ -889,24 +531,14 @@ TabController NSViewController - YES - - YES - closeTab: - commandDispatch: - - - YES - id - id - + closeTab: + id YES - + YES closeButton_ - contextMenu_ iconView_ target_ titleView_ @@ -914,7 +546,6 @@ YES HoverCloseButton - NSMenu NSView id NSTextField @@ -930,7 +561,7 @@ BackgroundGradientView YES - + YES closeButton_ controller_ @@ -947,695 +578,8 @@ - - YES - - NSActionCell - NSCell - - IBFrameworkSource - AppKit.framework/Headers/NSActionCell.h - - - - NSApplication - NSResponder - - IBFrameworkSource - AppKit.framework/Headers/NSApplication.h - - - - NSApplication - - IBFrameworkSource - AppKit.framework/Headers/NSApplicationScripting.h - - - - NSApplication - - IBFrameworkSource - AppKit.framework/Headers/NSColorPanel.h - - - - NSApplication - - IBFrameworkSource - AppKit.framework/Headers/NSHelpManager.h - - - - NSApplication - - IBFrameworkSource - AppKit.framework/Headers/NSPageLayout.h - - - - NSButton - NSControl - - IBFrameworkSource - AppKit.framework/Headers/NSButton.h - - - - NSButtonCell - NSActionCell - - IBFrameworkSource - AppKit.framework/Headers/NSButtonCell.h - - - - NSCell - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSCell.h - - - - NSControl - NSView - - IBFrameworkSource - AppKit.framework/Headers/NSControl.h - - - - NSFormatter - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSFormatter.h - - - - NSImageCell - NSCell - - IBFrameworkSource - AppKit.framework/Headers/NSImageCell.h - - - - NSImageView - NSControl - - IBFrameworkSource - AppKit.framework/Headers/NSImageView.h - - - - NSMenu - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSMenu.h - - - - NSMenuItem - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSMenuItem.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSAccessibility.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSAlert.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSAnimation.h - - - - NSObject - - - - NSObject - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSBrowser.h - - - - NSObject - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSComboBox.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSComboBoxCell.h - - - - NSObject - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSDatePickerCell.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSDictionaryController.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSDragging.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSDrawer.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSFontManager.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSFontPanel.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSImage.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSKeyValueBinding.h - - - - NSObject - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSNibLoading.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSOutlineView.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSPasteboard.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSRuleEditor.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSSavePanel.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSSound.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSSpeechRecognizer.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSSpeechSynthesizer.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSSplitView.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSTabView.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSTableView.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSText.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSTextStorage.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSTextView.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSTokenField.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSTokenFieldCell.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSToolbar.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSToolbarItem.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSView.h - - - - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSWindow.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSArchiver.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSClassDescription.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSConnection.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSError.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSFileManager.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSKeyValueCoding.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSKeyValueObserving.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSKeyedArchiver.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSMetadata.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSNetServices.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSObject.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSObjectScripting.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSPort.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSPortCoder.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSRunLoop.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSScriptClassDescription.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSScriptKeyValueCoding.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSScriptObjectSpecifiers.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSScriptWhoseTests.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSSpellServer.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSStream.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSThread.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSURL.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSURLConnection.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSURLDownload.h - - - - NSObject - - IBFrameworkSource - Foundation.framework/Headers/NSXMLParser.h - - - - NSObject - - IBFrameworkSource - Print.framework/Headers/PDEPluginInterface.h - - - - NSObject - - IBFrameworkSource - QuartzCore.framework/Headers/CAAnimation.h - - - - NSObject - - IBFrameworkSource - QuartzCore.framework/Headers/CALayer.h - - - - NSObject - - IBFrameworkSource - QuartzCore.framework/Headers/CIImageProvider.h - - - - NSObject - - IBFrameworkSource - SecurityInterface.framework/Headers/SFAuthorizationView.h - - - - NSObject - - IBFrameworkSource - SecurityInterface.framework/Headers/SFCertificatePanel.h - - - - NSObject - - IBFrameworkSource - SecurityInterface.framework/Headers/SFChooseIdentityPanel.h - - - - NSResponder - - IBFrameworkSource - AppKit.framework/Headers/NSInterfaceStyle.h - - - - NSResponder - NSObject - - IBFrameworkSource - AppKit.framework/Headers/NSResponder.h - - - - NSTextField - NSControl - - IBFrameworkSource - AppKit.framework/Headers/NSTextField.h - - - - NSTextFieldCell - NSActionCell - - IBFrameworkSource - AppKit.framework/Headers/NSTextFieldCell.h - - - - NSView - - IBFrameworkSource - AppKit.framework/Headers/NSClipView.h - - - - NSView - - - - NSView - - IBFrameworkSource - AppKit.framework/Headers/NSRulerView.h - - - - NSView - NSResponder - - - - NSViewController - NSResponder - - view - NSView - - - IBFrameworkSource - AppKit.framework/Headers/NSViewController.h - - - 0 - - com.apple.InterfaceBuilder.CocoaPlugin.macosx - - - - com.apple.InterfaceBuilder.CocoaPlugin.macosx - - - - com.apple.InterfaceBuilder.CocoaPlugin.InterfaceBuilder3 - - - YES ../../chrome.xcodeproj 3 diff --git a/chrome/browser/app_menu_model.h b/chrome/browser/app_menu_model.h index 086d613..d298a86 100644 --- a/chrome/browser/app_menu_model.h +++ b/chrome/browser/app_menu_model.h @@ -28,8 +28,6 @@ class AppMenuModel : public menus::SimpleMenuModel, private: void Build(); - // The top-level model. - scoped_ptr model_; // Contents of the profiles menu to populate with profile names. scoped_ptr profiles_menu_contents_; diff --git a/chrome/browser/app_menu_model_unittest.cc b/chrome/browser/app_menu_model_unittest.cc index b354292..1200c63 100644 --- a/chrome/browser/app_menu_model_unittest.cc +++ b/chrome/browser/app_menu_model_unittest.cc @@ -31,6 +31,27 @@ class Delegate : public menus::SimpleMenuModel::Delegate { class AppMenuModelTest : public BrowserWithTestWindowTest { }; +// Recursively checks the enabled state and executes a command on every item +// that's not a separator or a submenu parent item. The returned count should +// match the number of times the delegate is called to ensure every item works. +static void CountEnabledExecutable(menus::MenuModel* model, int* count) { + for (int i = 0; i < model->GetItemCount(); ++i) { + menus::MenuModel::ItemType type = model->GetTypeAt(i); + switch (type) { + case menus::MenuModel::TYPE_SEPARATOR: + continue; + case menus::MenuModel::TYPE_SUBMENU: + CountEnabledExecutable(model->GetSubmenuModelAt(i), count); + break; + default: + model->IsEnabledAt(i); // Check if it's enabled (ignore answer). + model->ActivatedAt(i); // Execute it. + (*count)++; // Increment the count of executable items seen. + break; + } + } +} + TEST_F(AppMenuModelTest, Basics) { Delegate delegate; AppMenuModel model(&delegate, browser()); @@ -39,14 +60,9 @@ TEST_F(AppMenuModelTest, Basics) { // the exact number. EXPECT_GT(model.GetItemCount(), 5); - // Execute a couple of the items and make sure it gets back to our delegate. - model.ActivatedAt(0); - EXPECT_TRUE(model.IsEnabledAt(0)); - model.ActivatedAt(3); - EXPECT_TRUE(model.IsEnabledAt(4)); - EXPECT_EQ(delegate.execute_count_, 2); - EXPECT_EQ(delegate.enable_count_, 2); - - delegate.execute_count_ = 0; - delegate.enable_count_ = 0; + int item_count = 0; + CountEnabledExecutable(&model, &item_count); + EXPECT_GT(item_count, 0); + EXPECT_EQ(item_count, delegate.execute_count_); + EXPECT_EQ(item_count, delegate.enable_count_); } diff --git a/chrome/browser/cocoa/menu_controller.mm b/chrome/browser/cocoa/menu_controller.mm index b63b663..7706d38 100644 --- a/chrome/browser/cocoa/menu_controller.mm +++ b/chrome/browser/cocoa/menu_controller.mm @@ -96,7 +96,8 @@ // The MenuModel works on indexes so we can't just set the command id as the // tag like we do in other menus. Also set the represented object to be // the model so hierarchical menus check the correct index in the correct - // model. + // model. Setting the target to |self| allows this class to participate + // in validation of the menu items. [item setTag:modelIndex]; [item setTarget:self]; NSValue* modelObject = [NSValue valueWithPointer:model]; diff --git a/chrome/browser/cocoa/tab_controller.h b/chrome/browser/cocoa/tab_controller.h index 5357544..75f51267 100644 --- a/chrome/browser/cocoa/tab_controller.h +++ b/chrome/browser/cocoa/tab_controller.h @@ -7,6 +7,7 @@ #import #import "chrome/browser/cocoa/hover_close_button.h" +#include "chrome/browser/tab_menu_model.h" // The loading/waiting state of the tab. // TODO(pinkerton): this really doesn't belong here, but something needs to @@ -20,6 +21,10 @@ enum TabLoadingState { kTabCrashed, }; +@class MenuController; +namespace TabControllerInternal { +class MenuDelegate; +} @class TabView; @protocol TabControllerTarget; @@ -38,7 +43,6 @@ enum TabLoadingState { @private IBOutlet NSView* iconView_; IBOutlet NSTextField* titleView_; - IBOutlet NSMenu* contextMenu_; IBOutlet HoverCloseButton* closeButton_; NSRect originalIconFrame_; // frame of iconView_ as loaded from nib @@ -50,6 +54,9 @@ enum TabLoadingState { CGFloat titleCloseWidthOffset_; // between right edges of icon and close btn. id target_; // weak, where actions are sent SEL action_; // selector sent when tab is selected by clicking + scoped_ptr contextMenuModel_; + scoped_ptr contextMenuDelegate_; + scoped_nsobject contextMenuController_; } @property(assign, nonatomic) TabLoadingState loadingState; @@ -74,9 +81,6 @@ enum TabLoadingState { // perform the close. - (IBAction)closeTab:(id)sender; -// Dispatches the command in the tag to the registered target object. -- (IBAction)commandDispatch:(id)sender; - // Replace the current icon view with the given view. |iconView| will be // resized to the size of the current icon view. - (void)setIconView:(NSView*)iconView; diff --git a/chrome/browser/cocoa/tab_controller.mm b/chrome/browser/cocoa/tab_controller.mm index 8488ea8..49ce170 100644 --- a/chrome/browser/cocoa/tab_controller.mm +++ b/chrome/browser/cocoa/tab_controller.mm @@ -4,6 +4,7 @@ #include "app/l10n_util_mac.h" #include "base/mac_util.h" +#import "chrome/browser/cocoa/menu_controller.h" #import "chrome/browser/cocoa/tab_controller.h" #import "chrome/browser/cocoa/tab_controller_target.h" #import "chrome/browser/cocoa/tab_view.h" @@ -17,6 +18,53 @@ @synthesize target = target_; @synthesize action = action_; +namespace TabControllerInternal { + +// A C++ delegate that handles enabling/disabling menu items and handling when +// a menu command is chosen. Also fixes up the menu item label for "pin/unpin +// tab". +class MenuDelegate : public menus::SimpleMenuModel::Delegate { + public: + explicit MenuDelegate(id target, TabController* owner) + : target_(target), owner_(owner) { } + + // Overridden from menus::SimpleMenuModel::Delegate + virtual bool IsCommandIdChecked(int command_id) const { return false; } + virtual bool IsCommandIdEnabled(int command_id) const { + TabStripModel::ContextMenuCommand command = + static_cast(command_id); + return [target_ isCommandEnabled:command forController:owner_]; + } + virtual bool GetAcceleratorForCommandId( + int command_id, + menus::Accelerator* accelerator) { return false; } + virtual void ExecuteCommand(int command_id) { + TabStripModel::ContextMenuCommand command = + static_cast(command_id); + [target_ commandDispatch:command forController:owner_]; + } + + virtual bool IsLabelForCommandIdDynamic(int command_id) const { + return command_id == TabStripModel::CommandTogglePinned; + } + virtual string16 GetLabelForCommandId(int command_id) const { + // Display "Pin Tab" when the tab is not pinned and "Unpin Tab" when it is + // (this is not a checkmark menu item, per Apple's HIG). + if (command_id == TabStripModel::CommandTogglePinned) { + return l10n_util::GetStringUTF16( + [owner_ pinned] ? IDS_TAB_CXMENU_UNPIN_TAB_MAC + : IDS_TAB_CXMENU_PIN_TAB_MAC); + } + return string16(); + } + + private: + id target_; // weak + TabController* owner_; // weak, owns me +}; + +} // namespace + // The min widths match the windows values and are sums of left + right // padding, of which we have no comparable constants (we draw using paths, not // images). The selected tab width includes the close button width. @@ -79,6 +127,19 @@ [self internalSetSelected:selected_]; } +// Called when Cocoa wants to display the context menu. Lazily instantiate +// the menu based off of the cross-platform model. Re-create the menu and +// model every time to get the correct labels and enabling. +- (NSMenu*)menu { + contextMenuDelegate_.reset( + new TabControllerInternal::MenuDelegate(target_, self)); + contextMenuModel_.reset(new TabMenuModel(contextMenuDelegate_.get())); + contextMenuController_.reset( + [[MenuController alloc] initWithModel:contextMenuModel_.get() + useWithPopUpButtonCell:NO]); + return [contextMenuController_ menu]; +} + - (IBAction)closeTab:(id)sender { if ([[self target] respondsToSelector:@selector(closeTab:)]) { [[self target] performSelector:@selector(closeTab:) @@ -86,22 +147,6 @@ } } -// Dispatches the command in the tag to the registered target object. -- (IBAction)commandDispatch:(id)sender { - TabStripModel::ContextMenuCommand command = - static_cast([sender tag]); - [[self target] commandDispatch:command forController:self]; -} - -// Called for each menu item on its target, which would be this controller. -// Returns YES if the menu item should be enabled. We ask the tab's -// target for the proper answer. -- (BOOL)validateMenuItem:(NSMenuItem*)item { - TabStripModel::ContextMenuCommand command = - static_cast([item tag]); - return [[self target] isCommandEnabled:command forController:self]; -} - - (void)setTitle:(NSString*)title { [[self view] setToolTip:title]; [super setTitle:title]; @@ -259,16 +304,4 @@ return NO; } -// Delegate method for context menu. Called before the menu is displayed to -// update the menu. We need to display "Pin Tab" when the tab is not pinned and -// "Unpin Tab" when it is (this is not a checkmark menu item, per Apple's HIG). -- (void)menuNeedsUpdate:(NSMenu*)menu { - NSMenuItem* togglePinned = - [menu itemWithTag:TabStripModel::CommandTogglePinned]; - NSString* menuItemText = l10n_util::GetNSStringWithFixup( - [self pinned] ? IDS_TAB_CXMENU_UNPIN_TAB_MAC - : IDS_TAB_CXMENU_PIN_TAB_MAC); - [togglePinned setTitle:menuItemText]; -} - @end diff --git a/chrome/browser/cocoa/tab_controller_unittest.mm b/chrome/browser/cocoa/tab_controller_unittest.mm index 26484b5..bab2bef 100644 --- a/chrome/browser/cocoa/tab_controller_unittest.mm +++ b/chrome/browser/cocoa/tab_controller_unittest.mm @@ -257,4 +257,17 @@ TEST_F(TabControllerTest, ShouldShowIcon) { EXPECT_GT(cap, 0); } +TEST_F(TabControllerTest, Menu) { + NSWindow* window = test_window(); + scoped_nsobject controller([[TabController alloc] init]); + [[window contentView] addSubview:[controller view]]; + int cap = [controller iconCapacity]; + EXPECT_GT(cap, 0); + + // Asking the view for its menu should yield a valid menu. + NSMenu* menu = [[controller view] menu]; + EXPECT_TRUE(menu); + EXPECT_GT([menu numberOfItems], 0); +} + } // namespace diff --git a/chrome/browser/cocoa/tab_view.mm b/chrome/browser/cocoa/tab_view.mm index dbf9e78..28dca36 100644 --- a/chrome/browser/cocoa/tab_view.mm +++ b/chrome/browser/cocoa/tab_view.mm @@ -54,6 +54,12 @@ const CGFloat kRapidCloseDist = 2.5; [super dealloc]; } +// Use the TabController to provide the menu rather than obtaining it from the +// nib file. +- (NSMenu*)menu { + return [controller_ menu]; +} + // Overridden so that mouse clicks come to this view (the parent of the // hierarchy) first. We want to handle clicks and drags in this class and // leave the background button for display purposes only. diff --git a/chrome/browser/cocoa/tab_view_unittest.mm b/chrome/browser/cocoa/tab_view_unittest.mm index 18b3fe5..0c4d768 100644 --- a/chrome/browser/cocoa/tab_view_unittest.mm +++ b/chrome/browser/cocoa/tab_view_unittest.mm @@ -41,4 +41,9 @@ TEST_F(TabViewTest, MouseTracking) { // TODO(pinkerton): Test dragging out of window } +// Test it doesn't crash when asked for its menu with no TabController set. +TEST_F(TabViewTest, Menu) { + EXPECT_FALSE([view_ menu]); +} + } // namespace diff --git a/chrome/browser/page_menu_model.h b/chrome/browser/page_menu_model.h index b3fc8ed..59994a9 100644 --- a/chrome/browser/page_menu_model.h +++ b/chrome/browser/page_menu_model.h @@ -67,10 +67,9 @@ class PageMenuModel : public menus::SimpleMenuModel { private: void Build(); - // The top-level model. - scoped_ptr model_; - // Models for submenus referenced by model_. SimpleMenuModel only uses weak - // references so these must be kept for the lifetime of the top-level model. + // Models for submenus referenced by this model. SimpleMenuModel only uses + // weak references so these must be kept for the lifetime of the top-level + // model. scoped_ptr zoom_menu_model_; scoped_ptr encoding_menu_model_; scoped_ptr devtools_menu_model_; diff --git a/chrome/browser/tab_menu_model.cc b/chrome/browser/tab_menu_model.cc new file mode 100644 index 0000000..f426250 --- /dev/null +++ b/chrome/browser/tab_menu_model.cc @@ -0,0 +1,43 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. Use of this +// source code is governed by a BSD-style license that can be found in the +// LICENSE file. + +#include "chrome/browser/tab_menu_model.h" + +#include "chrome/browser/tabs/tab_strip_model.h" +#include "grit/generated_resources.h" + +TabMenuModel::TabMenuModel(menus::SimpleMenuModel::Delegate* delegate) + : menus::SimpleMenuModel(delegate) { + Build(); +} + +void TabMenuModel::Build() { + AddItemWithStringId(TabStripModel::CommandNewTab, IDS_TAB_CXMENU_NEWTAB); + AddSeparator(); + AddItemWithStringId(TabStripModel::CommandReload, IDS_TAB_CXMENU_RELOAD); + AddItemWithStringId(TabStripModel::CommandDuplicate, + IDS_TAB_CXMENU_DUPLICATE); + // On Mac the HIG prefers "pin/unpin" to a checkmark. The Mac code will fix up + // the actual string based on the tab's state via the delegate. +#if defined(OS_MACOSX) + AddItemWithStringId(TabStripModel::CommandTogglePinned, + IDS_TAB_CXMENU_PIN_TAB); +#else + AddCheckItemWithStringId(TabStripModel::CommandTogglePinned, + IDS_TAB_CXMENU_PIN_TAB); +#endif + AddSeparator(); + AddItemWithStringId(TabStripModel::CommandCloseTab, + IDS_TAB_CXMENU_CLOSETAB); + AddItemWithStringId(TabStripModel::CommandCloseOtherTabs, + IDS_TAB_CXMENU_CLOSEOTHERTABS); + AddItemWithStringId(TabStripModel::CommandCloseTabsToRight, + IDS_TAB_CXMENU_CLOSETABSTORIGHT); + AddItemWithStringId(TabStripModel::CommandCloseTabsOpenedBy, + IDS_TAB_CXMENU_CLOSETABSOPENEDBY); + AddSeparator(); + AddItemWithStringId(TabStripModel::CommandRestoreTab, IDS_RESTORE_TAB); + AddItemWithStringId(TabStripModel::CommandBookmarkAllTabs, + IDS_TAB_CXMENU_BOOKMARK_ALL_TABS); +} diff --git a/chrome/browser/tab_menu_model.h b/chrome/browser/tab_menu_model.h new file mode 100644 index 0000000..d176fc9 --- /dev/null +++ b/chrome/browser/tab_menu_model.h @@ -0,0 +1,25 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. Use of this +// source code is governed by a BSD-style license that can be found in the +// LICENSE file. + +#ifndef CHROME_BROWSER_TAB_MENU_MODEL_H_ +#define CHROME_BROWSER_TAB_MENU_MODEL_H_ + +#include "app/menus/simple_menu_model.h" + +class Browser; + +// A menu model that builds the contents of the tab context menu. This menu has +// only one level (no submenus). +class TabMenuModel : public menus::SimpleMenuModel { + public: + explicit TabMenuModel(menus::SimpleMenuModel::Delegate* delegate); + virtual ~TabMenuModel() { } + + private: + void Build(); + + DISALLOW_COPY_AND_ASSIGN(TabMenuModel); +}; + +#endif // CHROME_BROWSER_TAB_MENU_MODEL_H_ diff --git a/chrome/browser/tab_menu_model_unittest.cc b/chrome/browser/tab_menu_model_unittest.cc new file mode 100644 index 0000000..eb9aa65 --- /dev/null +++ b/chrome/browser/tab_menu_model_unittest.cc @@ -0,0 +1,68 @@ +// Copyright (c) 2009 The Chromium Authors. All rights reserved. Use of this +// source code is governed by a BSD-style license that can be found in the +// LICENSE file. + +#include "chrome/browser/tab_menu_model.h" + +#include "base/logging.h" +#include "testing/gtest/include/gtest/gtest.h" +#include "testing/platform_test.h" + +// A menu delegate that counts the number of times certain things are called +// to make sure things are hooked up properly. +class Delegate : public menus::SimpleMenuModel::Delegate { + public: + Delegate() : execute_count_(0), enable_count_(0) { } + + virtual bool IsCommandIdChecked(int command_id) const { return false; } + virtual bool IsCommandIdEnabled(int command_id) const { + ++enable_count_; + return true; + } + virtual bool GetAcceleratorForCommandId( + int command_id, + menus::Accelerator* accelerator) { return false; } + virtual void ExecuteCommand(int command_id) { ++execute_count_; } + + int execute_count_; + mutable int enable_count_; +}; + +class TabMenuModelTest : public PlatformTest { +}; + +// Recursively checks the enabled state and executes a command on every item +// that's not a separator or a submenu parent item. The returned count should +// match the number of times the delegate is called to ensure every item works. +static void CountEnabledExecutable(menus::MenuModel* model, int* count) { + for (int i = 0; i < model->GetItemCount(); ++i) { + menus::MenuModel::ItemType type = model->GetTypeAt(i); + switch (type) { + case menus::MenuModel::TYPE_SEPARATOR: + continue; + case menus::MenuModel::TYPE_SUBMENU: + CountEnabledExecutable(model->GetSubmenuModelAt(i), count); + break; + default: + model->IsEnabledAt(i); // Check if it's enabled (ignore answer). + model->ActivatedAt(i); // Execute it. + (*count)++; // Increment the count of executable items seen. + break; + } + } +} + +TEST_F(TabMenuModelTest, Basics) { + Delegate delegate; + TabMenuModel model(&delegate); + + // Verify it has items. The number varies by platform, so we don't check + // the exact number. + EXPECT_GT(model.GetItemCount(), 5); + + int item_count = 0; + CountEnabledExecutable(&model, &item_count); + EXPECT_GT(item_count, 0); + EXPECT_EQ(item_count, delegate.execute_count_); + EXPECT_EQ(item_count, delegate.enable_count_); +} diff --git a/chrome/browser/views/tabs/tab.cc b/chrome/browser/views/tabs/tab.cc index 7140ff0..a775734 100644 --- a/chrome/browser/views/tabs/tab.cc +++ b/chrome/browser/views/tabs/tab.cc @@ -12,6 +12,7 @@ #include "app/resource_bundle.h" #include "base/compiler_specific.h" #include "base/gfx/size.h" +#include "chrome/browser/tab_menu_model.h" #include "chrome/browser/views/frame/browser_extender.h" #include "chrome/browser/views/frame/browser_view.h" #include "chrome/browser/views/tabs/tab_strip.h" @@ -26,11 +27,10 @@ static const SkScalar kTabCapWidth = 15; static const SkScalar kTabTopCurveWidth = 4; static const SkScalar kTabBottomCurveWidth = 3; -class Tab::TabContextMenuContents : public menus::SimpleMenuModel, - public menus::SimpleMenuModel::Delegate { +class Tab::TabContextMenuContents : public menus::SimpleMenuModel::Delegate { public: explicit TabContextMenuContents(Tab* tab) - : ALLOW_THIS_IN_INITIALIZER_LIST(menus::SimpleMenuModel(this)), + : ALLOW_THIS_IN_INITIALIZER_LIST(model_(this)), tab_(tab), last_command_(TabStripModel::CommandFirst) { Build(); @@ -84,29 +84,10 @@ class Tab::TabContextMenuContents : public menus::SimpleMenuModel, private: void Build() { - AddItemWithStringId(TabStripModel::CommandNewTab, IDS_TAB_CXMENU_NEWTAB); - AddSeparator(); - AddItemWithStringId(TabStripModel::CommandReload, IDS_TAB_CXMENU_RELOAD); - AddItemWithStringId(TabStripModel::CommandDuplicate, - IDS_TAB_CXMENU_DUPLICATE); - AddCheckItemWithStringId(TabStripModel::CommandTogglePinned, - IDS_TAB_CXMENU_PIN_TAB); - AddSeparator(); - AddItemWithStringId(TabStripModel::CommandCloseTab, - IDS_TAB_CXMENU_CLOSETAB); - AddItemWithStringId(TabStripModel::CommandCloseOtherTabs, - IDS_TAB_CXMENU_CLOSEOTHERTABS); - AddItemWithStringId(TabStripModel::CommandCloseTabsToRight, - IDS_TAB_CXMENU_CLOSETABSTORIGHT); - AddItemWithStringId(TabStripModel::CommandCloseTabsOpenedBy, - IDS_TAB_CXMENU_CLOSETABSOPENEDBY); - AddSeparator(); - AddItemWithStringId(TabStripModel::CommandRestoreTab, IDS_RESTORE_TAB); - AddItemWithStringId(TabStripModel::CommandBookmarkAllTabs, - IDS_TAB_CXMENU_BOOKMARK_ALL_TABS); - menu_.reset(new views::Menu2(this)); + menu_.reset(new views::Menu2(&model_)); } + TabMenuModel model_; scoped_ptr menu_; // The Tab the context menu was brought up for. Set to NULL when the menu diff --git a/chrome/chrome_browser.gypi b/chrome/chrome_browser.gypi index 8c98e0c..2791031 100755 --- a/chrome/chrome_browser.gypi +++ b/chrome/chrome_browser.gypi @@ -1518,6 +1518,8 @@ 'browser/tab_contents/web_drag_dest_gtk.h', 'browser/tab_contents/web_drop_target_win.cc', 'browser/tab_contents/web_drop_target_win.h', + 'browser/tab_menu_model.cc', + 'browser/tab_menu_model.h', 'browser/tabs/tab_strip_model.cc', 'browser/tabs/tab_strip_model.h', 'browser/tabs/tab_strip_model_order_controller.cc', diff --git a/chrome/chrome_tests.gypi b/chrome/chrome_tests.gypi index 5883286..c25bf49 100755 --- a/chrome/chrome_tests.gypi +++ b/chrome/chrome_tests.gypi @@ -778,6 +778,7 @@ 'browser/tab_contents/render_view_host_manager_unittest.cc', 'browser/tab_contents/thumbnail_generator_unittest.cc', 'browser/tab_contents/web_contents_unittest.cc', + 'browser/tab_menu_model_unittest.cc', 'browser/tabs/tab_strip_model_unittest.cc', 'browser/task_manager_unittest.cc', 'browser/theme_resources_util_unittest.cc', -- cgit v1.1