From 0a7e7db5e1b3037356308617b04e163c28069a75 Mon Sep 17 00:00:00 2001 From: "cbarrett%mozilla.com" Date: Tue, 28 Aug 2007 00:57:49 +0000 Subject: [PATCH] Bug 382194 - Remove an assertion in nsMenuX, clean up native menu code. r=joshmoz sr=pavlov --- widget/public/nsIMenu.h | 25 ++- widget/src/cocoa/nsMenuX.h | 4 +- widget/src/cocoa/nsMenuX.mm | 319 ++++++++++++++++++++++-------------- 3 files changed, 215 insertions(+), 133 deletions(-) diff --git a/widget/public/nsIMenu.h b/widget/public/nsIMenu.h index 664312d4fdf..bc6af85099c 100644 --- a/widget/public/nsIMenu.h +++ b/widget/public/nsIMenu.h @@ -51,11 +51,10 @@ class nsIContent; class nsIMenuCommandDispatcher; -// {FC5BCA9C-4494-4C0F-BEFD-CB31BEBA1531} +// a0aa02c5-5549-4228-8514-1083c6e8aa5d #define NS_IMENU_IID \ -{ 0xFC5BCA9C, 0x4494, 0x4C0F, \ - { 0xBE, 0xFD, 0xCB, 0x31, 0xBE, 0xBA, 0x15, 0x31 } } - +{ 0xa0aa02c5, 0x5549, 0x4228, \ + { 0x85, 0x14, 0x10, 0x83, 0xc6, 0xe8, 0xaa, 0x5d } } /** * Menu widget @@ -129,15 +128,29 @@ class nsIMenu : public nsISupports { NS_IMETHOD AddSeparator() = 0; /** - * Returns the number of menu items + * Returns the number of visible menu items * This includes separators. It does not include hidden items. * */ + NS_IMETHOD GetVisibleItemCount(PRUint32 &aCount) = 0; + + /** + * Returns a Menu or Menu Item at a specified Index. + * This includes separators. It does not include hidden items. + * + */ + NS_IMETHOD GetVisibleItemAt(const PRUint32 aPos, nsISupports *& aMenuItem) = 0; + + /** + * Returns the number of menu items + * This includes separators. It -does- include hidden items. + * + */ NS_IMETHOD GetItemCount(PRUint32 &aCount) = 0; /** * Returns a Menu or Menu Item at a specified Index. - * This includes separators. It does not include hidden items. + * This includes separators. It -does- include hidden items. * */ NS_IMETHOD GetItemAt(const PRUint32 aPos, nsISupports *& aMenuItem) = 0; diff --git a/widget/src/cocoa/nsMenuX.h b/widget/src/cocoa/nsMenuX.h index 6b32db7260b..7407ec07a1f 100644 --- a/widget/src/cocoa/nsMenuX.h +++ b/widget/src/cocoa/nsMenuX.h @@ -106,6 +106,8 @@ public: NS_IMETHOD AddSeparator(); NS_IMETHOD GetItemCount(PRUint32 &aCount); NS_IMETHOD GetItemAt(const PRUint32 aPos, nsISupports *& aMenuItem); + NS_IMETHOD GetVisibleItemCount(PRUint32 &aCount); + NS_IMETHOD GetVisibleItemAt(const PRUint32 aPos, nsISupports *& aMenuItem); NS_IMETHOD InsertItemAt(const PRUint32 aPos, nsISupports * aMenuItem); NS_IMETHOD RemoveItem(const PRUint32 aPos); NS_IMETHOD RemoveAll(); @@ -149,7 +151,7 @@ protected: protected: nsString mLabel; nsCOMArray mMenuItemsArray; - nsCOMArray mHiddenMenuItemsArray; + PRUint32 mVisibleItemsCount; // caching number of visible items in mMenuItemsArray nsISupports* mParent; // weak, my parent owns me nsIChangeManager* mManager; // weak ref, it will outlive us [menubar] diff --git a/widget/src/cocoa/nsMenuX.mm b/widget/src/cocoa/nsMenuX.mm index e2e5938abb7..de61df8fb52 100644 --- a/widget/src/cocoa/nsMenuX.mm +++ b/widget/src/cocoa/nsMenuX.mm @@ -106,10 +106,10 @@ NS_IMPL_ISUPPORTS4(nsMenuX, nsIMenu, nsIMenuListener, nsIChangeObserver, nsISupp nsMenuX::nsMenuX() -: mParent(nsnull), mManager(nsnull), mMacMenuID(0), mMacMenu(nil), mNativeMenuItem(nil), - mIsEnabled(PR_TRUE), mDestroyHandlerCalled(PR_FALSE), - mNeedsRebuild(PR_TRUE), mConstructed(PR_FALSE), mVisible(PR_TRUE), - mXBLAttached(PR_FALSE) +: mVisibleItemsCount(0), mParent(nsnull), mManager(nsnull), mMacMenuID(0), + mMacMenu(nil), mNativeMenuItem(nil), mIsEnabled(PR_TRUE), + mDestroyHandlerCalled(PR_FALSE), mNeedsRebuild(PR_TRUE), + mConstructed(PR_FALSE), mVisible(PR_TRUE), mXBLAttached(PR_FALSE) { mMenuDelegate = [[MenuDelegate alloc] initWithGeckoMenu:this]; @@ -253,14 +253,11 @@ nsresult nsMenuX::AddMenuItem(nsIMenuItem * aMenuItem) nsCOMPtr menuItemContent; aMenuItem->GetMenuItemContent(getter_AddRefs(menuItemContent)); - if (menuItemContent && NodeIsHiddenOrCollapsed(menuItemContent)) { - mHiddenMenuItemsArray.AppendObject(aMenuItem); // owning ref + mMenuItemsArray.AppendObject(aMenuItem); // owning ref + if (menuItemContent && NodeIsHiddenOrCollapsed(menuItemContent)) return NS_OK; - } - else { - mMenuItemsArray.AppendObject(aMenuItem); // owning ref - } - + ++mVisibleItemsCount; + // add the menu item to this menu NSMenuItem* newNativeMenuItem; aMenuItem->GetNativeData((void*&)newNativeMenuItem); @@ -293,13 +290,10 @@ nsresult nsMenuX::AddMenu(nsIMenu * aMenu) nsCOMPtr menuContent; aMenu->GetMenuContent(getter_AddRefs(menuContent)); - if (menuContent && NodeIsHiddenOrCollapsed(menuContent)) { - mHiddenMenuItemsArray.AppendObject(aMenu); // owning ref + mMenuItemsArray.AppendObject(aMenu); // owning ref + if (menuContent && NodeIsHiddenOrCollapsed(menuContent)) return NS_OK; - } - else { - mMenuItemsArray.AppendObject(aMenu); // owning ref - } + ++mVisibleItemsCount; // We have to add a menu item and then associate the menu with it NSMenuItem* newNativeMenuItem = (static_cast(aMenu))->GetNativeMenuItem(); @@ -320,11 +314,13 @@ NS_IMETHODIMP nsMenuX::AddSeparator() // We're not really appending an nsMenuItem but a placeholder needs to be // here to make sure that event dispatching isn't off by one. mMenuItemsArray.AppendObject(&gDummyMenuItemX); // owning ref + ++mVisibleItemsCount; [mMacMenu addItem:[NSMenuItem separatorItem]]; return NS_OK; } +// Includes all items, including hidden/collapsed ones NS_IMETHODIMP nsMenuX::GetItemCount(PRUint32 &aCount) { aCount = mMenuItemsArray.Count(); @@ -332,14 +328,82 @@ NS_IMETHODIMP nsMenuX::GetItemCount(PRUint32 &aCount) } +// Includes all items, including hidden/collapsed ones NS_IMETHODIMP nsMenuX::GetItemAt(const PRUint32 aPos, nsISupports *& aMenuItem) { + if (aPos >= (PRUint32)mMenuItemsArray.Count()) + return NS_ERROR_INVALID_ARG; + aMenuItem = mMenuItemsArray.ObjectAt(aPos); NS_IF_ADDREF(aMenuItem); return NS_OK; } +// Checks both nsIMenus and nsIMenuItems. Not suitable for menus that are children +// of nsIMenuBar, which has slightly different rules for visiblity. +static PRBool MenuNodeIsVisible(nsISupports *item) +{ + // Find the content for this item in the menu, be it a MenuItem or a Menu + nsCOMPtr itemContent; + nsCOMPtr menuItem = do_QueryInterface(item); + if (menuItem) + menuItem->GetMenuItemContent(getter_AddRefs(itemContent)); + else { + nsCOMPtr menu = do_QueryInterface(item); + if (menu) + menu->GetMenuContent(getter_AddRefs(itemContent)); + } + + // Check the visibility of the item's content + return (itemContent && !NodeIsHiddenOrCollapsed(itemContent)); +} + + +// Only includes visible items +NS_IMETHODIMP nsMenuX::GetVisibleItemCount(PRUint32 &aCount) +{ + aCount = mVisibleItemsCount; + return NS_OK; +} + + +// Only includes visible items. Note that this is provides O(N) access +// If you need to iterate or search, consider using GetItemAt and doing your own filtering +NS_IMETHODIMP nsMenuX::GetVisibleItemAt(const PRUint32 aPos, nsISupports *& aMenuItem) +{ + PRUint32 count = mMenuItemsArray.Count(); + if (aPos >= mVisibleItemsCount || aPos >= count) + return NS_ERROR_INVALID_ARG; + + // If there are no invisible items, can provide direct access + if (mVisibleItemsCount == count) { + nsCOMPtr item = mMenuItemsArray.ObjectAt(aPos); + aMenuItem = item; + NS_IF_ADDREF(aMenuItem); + return NS_OK; + } + + // Otherwise, traverse the array until we find the the item we're looking for. + nsCOMPtr item; + PRUint32 visibleNodeIndex = 0; + for (PRUint32 i = 0; i < count; i++) { + item = mMenuItemsArray.ObjectAt(i); + if (MenuNodeIsVisible(item)) { + if (aPos == visibleNodeIndex) { + // we found the visible node we're looking for, return it + aMenuItem = item; + NS_IF_ADDREF(aMenuItem); + return NS_OK; + } + visibleNodeIndex++; + } + } + + return NS_ERROR_FAILURE; +} + + NS_IMETHODIMP nsMenuX::InsertItemAt(const PRUint32 aPos, nsISupports * aMenuItem) { return NS_ERROR_NOT_IMPLEMENTED; @@ -367,7 +431,7 @@ NS_IMETHODIMP nsMenuX::RemoveAll() } // get rid of Gecko menu items mMenuItemsArray.Clear(); - mHiddenMenuItemsArray.Clear(); + mVisibleItemsCount = 0; return NS_OK; } @@ -707,12 +771,8 @@ void nsMenuX::LoadSeparator(nsIContent* inSeparatorContent) } -// -// OnCreate -// // Fire our oncreate handler. Returns TRUE if we should keep processing the event, // FALSE if the handler wants to stop the creation of the menu -// PRBool nsMenuX::OnCreate() { nsEventStatus status = nsEventStatus_eIgnore; @@ -818,12 +878,8 @@ PRBool nsMenuX::OnCreated() } -// -// OnDestroy -// // Fire our ondestroy handler. Returns TRUE if we should keep processing the event, // FALSE if the handler wants to stop the destruction of the menu -// PRBool nsMenuX::OnDestroy() { if (mDestroyHandlerCalled) @@ -883,13 +939,9 @@ PRBool nsMenuX::OnDestroyed() } -// -// GetMenuPopupContent -// // Find the |menupopup| child in the |popup| representing this menu. It should be one // of a very few children so we won't be iterating over a bazillion menu items to find // it (so the strcmp won't kill us). -// void nsMenuX::GetMenuPopupContent(nsIContent** aResult) { if (!aResult) @@ -915,52 +967,69 @@ void nsMenuX::GetMenuPopupContent(nsIContent** aResult) } } -} // GetMenuPopupContent +} -// -// CountVisibleBefore -// // Determines how many menus are visible among the siblings that are before me. // It doesn't matter if I am visible. Note that this will always count the // Application menu, since we always put it in there. -// nsresult nsMenuX::CountVisibleBefore(PRUint32* outVisibleBefore) { NS_ASSERTION(outVisibleBefore, "bad index param in nsMenuX::CountVisibleBefore"); nsCOMPtr menubarParent = do_QueryInterface(mParent); - if (!menubarParent) - return NS_ERROR_FAILURE; - - PRUint32 numMenus = 0; - menubarParent->GetMenuCount(numMenus); + if (menubarParent) { + PRUint32 numMenus = 0; + menubarParent->GetMenuCount(numMenus); + + // Find this menu among the children of my parent menubar + *outVisibleBefore = 1; // start at 1, the Application menu will always be there + for (PRUint32 i = 0; i < numMenus; i++) { + nsCOMPtr currMenu; + menubarParent->GetMenuAt(i, *getter_AddRefs(currMenu)); + if (currMenu == static_cast(this)) { + // we found ourselves, break out + return NS_OK; + } - // Find this menu among the children of my parent menubar - PRBool gotThisMenu = PR_FALSE; - *outVisibleBefore = 1; // start at 1, the Application menu will always be there - for (PRUint32 i = 0; i < numMenus; i++) { - nsCOMPtr currMenu; - menubarParent->GetMenuAt(i, *getter_AddRefs(currMenu)); - if (currMenu == static_cast(this)) { - // we found ourselves, break out - gotThisMenu = PR_TRUE; - break; - } - - if (currMenu) { - nsCOMPtr menuContent; - currMenu->GetMenuContent(getter_AddRefs(menuContent)); - if (menuContent && - menuContent->GetChildCount() > 0 && - !NodeIsHiddenOrCollapsed(menuContent)) { - ++(*outVisibleBefore); + if (currMenu) { + nsCOMPtr menuContent; + currMenu->GetMenuContent(getter_AddRefs(menuContent)); + if (menuContent && + menuContent->GetChildCount() > 0 && + !NodeIsHiddenOrCollapsed(menuContent)) { + ++(*outVisibleBefore); + } } } - } + } // if menubarParent + else { + nsCOMPtr menuParent = do_QueryInterface(mParent); + if (!menuParent) + return NS_ERROR_FAILURE; - return gotThisMenu ? NS_OK : NS_ERROR_FAILURE; -} // CountVisibleBefore + PRUint32 numItems; + menuParent->GetItemCount(numItems); + + // Find this menu among the children of my parent menu + for (PRUint32 i = 0; i < numItems; i++) { + // Using GetItemAt instead of GetVisibleItemAt to avoid O(N^2) + nsCOMPtr currItem; + menuParent->GetItemAt(i, *getter_AddRefs(currItem)); + nsCOMPtr currMenu = do_QueryInterface(currItem); + if (currMenu == static_cast(this)) { + // we found ourselves, break out + return NS_OK; + } + + // If the node is visible increment the outparam. + if (MenuNodeIsVisible(currItem)) + ++(*outVisibleBefore); + + } + } + return NS_ERROR_FAILURE; +} NS_IMETHODIMP @@ -979,14 +1048,22 @@ nsMenuX::GetMenuRefAndItemIndexForMenuItem(nsISupports* aMenuItem, if (!mMacMenu) return NS_ERROR_FAILURE; - // look for the menu item given - PRUint32 menuItemCount = mMenuItemsArray.Count(); + // look for the menu item given, and skip invisible elements + PRUint32 menuItemCount; + GetItemCount(menuItemCount); + PRUint32 visibleNodeIndex = 0; for (PRUint32 i = 0; i < menuItemCount; i++) { - nsCOMPtr currItem = mMenuItemsArray.ObjectAt(i); - if (currItem == aMenuItem) { - *aMenuRef = _NSGetCarbonMenu(mMacMenu); - *aMenuItemIndex = i + 1; - return NS_OK; + nsCOMPtr currItem; + GetItemAt(i, *getter_AddRefs(currItem)); + // Only check visible nodes + if (MenuNodeIsVisible(currItem)) { + if (currItem == aMenuItem) { + *aMenuRef = _NSGetCarbonMenu(mMacMenu); + // add 1 because carbon menu items are 1-indexed. + *aMenuItemIndex = visibleNodeIndex + 1; + return NS_OK; + } + visibleNodeIndex++; } } @@ -1012,17 +1089,13 @@ NS_IMETHODIMP nsMenuX::AttributeChanged(nsIDocument *aDocument, PRInt32 aNameSpa // ignore the |open| attribute, which is by far the most common if (gConstructingMenu || (aAttribute == nsWidgetAtoms::open)) return NS_OK; - + nsCOMPtr menubarParent = do_QueryInterface(mParent); if (aAttribute == nsWidgetAtoms::disabled) { SetRebuild(PR_TRUE); - - if (mMenuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::disabled, - nsWidgetAtoms::_true, eCaseMatters)) - SetEnabled(PR_FALSE); - else - SetEnabled(PR_TRUE); + SetEnabled(!mMenuContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::disabled, + nsWidgetAtoms::_true, eCaseMatters)); } else if (aAttribute == nsWidgetAtoms::label) { SetRebuild(PR_TRUE); @@ -1048,59 +1121,48 @@ NS_IMETHODIMP nsMenuX::AttributeChanged(nsIDocument *aDocument, PRInt32 aNameSpa else if (aAttribute == nsWidgetAtoms::hidden || aAttribute == nsWidgetAtoms::collapsed) { SetRebuild(PR_TRUE); - if (NodeIsHiddenOrCollapsed(mMenuContent)) { - if (mVisible) { - if (menubarParent) { - PRUint32 indexToRemove = 0; - if (NS_SUCCEEDED(CountVisibleBefore(&indexToRemove))) { - void *clientData = nsnull; - menubarParent->GetNativeData(clientData); - if (clientData) { - NSMenu* menubar = reinterpret_cast(clientData); - [menubar removeItemAtIndex:indexToRemove]; - mVisible = PR_FALSE; - } - } - } // if on the menubar - else { - // hide this submenu - NS_ASSERTION(PR_FALSE, "nsMenuX::AttributeChanged: WRITE HIDE CODE FOR SUBMENU."); - } - } // if visible - else - NS_WARNING("You're hiding the menu twice, please stop"); - } // if told to hide menu + PRBool contentIsHiddenOrCollapsed = NodeIsHiddenOrCollapsed(mMenuContent); + + // don't do anything if the state is correct already + if (contentIsHiddenOrCollapsed != mVisible) + return NS_OK; + + nsCOMPtr menuParent = do_QueryInterface(mParent); + if (contentIsHiddenOrCollapsed) { + void *clientData = nsnull; + if (menubarParent) + menubarParent->GetNativeData(clientData); + else if (menuParent) + menuParent->GetNativeData(&clientData); + if (clientData) { + NSMenu* parentMenu = reinterpret_cast(clientData); + [parentMenu removeItem:mNativeMenuItem]; + mVisible = PR_FALSE; + } + } else { - if (!mVisible) { - if (menubarParent) { - PRUint32 insertAfter = 0; - if (NS_SUCCEEDED(CountVisibleBefore(&insertAfter))) { - void *clientData = nsnull; - menubarParent->GetNativeData(clientData); - if (clientData) { - NSMenu* menubar = reinterpret_cast(clientData); - // Shove this menu into its rightful place in the menubar. It doesn't matter - // what title we pass to InsertMenuItem() because when we stuff the actual menu - // handle in, the correct title goes with it. - [menubar insertItemWithTitle:@"placeholder" action:nil keyEquivalent:@"" atIndex:insertAfter]; - [[menubar itemAtIndex:insertAfter] setSubmenu:mMacMenu]; - mVisible = PR_TRUE; - } - } - } // if on menubar - else { - // show this submenu - NS_ASSERTION(PR_FALSE, "nsMenuX::AttributeChanged: WRITE SHOW CODE FOR SUBMENU."); + PRUint32 insertAfter = 0; + if (NS_SUCCEEDED(CountVisibleBefore(&insertAfter))) { + void *clientData = nsnull; + if (menubarParent) + menubarParent->GetNativeData(clientData); + else if (menuParent) + menuParent->GetNativeData(&clientData); + if (clientData) { + NSMenu* parentMenu = reinterpret_cast(clientData); + [parentMenu insertItem:mNativeMenuItem atIndex:insertAfter]; + [mNativeMenuItem setSubmenu:mMacMenu]; + mVisible = PR_TRUE; } - } // if not visible - } // if told to show menu + } + } } else if (aAttribute == nsWidgetAtoms::image) { SetupIcon(); } return NS_OK; -} // AttributeChanged +} NS_IMETHODIMP nsMenuX::ContentRemoved(nsIDocument *aDocument, nsIContent *aChild, @@ -1115,7 +1177,7 @@ NS_IMETHODIMP nsMenuX::ContentRemoved(nsIDocument *aDocument, nsIContent *aChild mManager->Unregister(aChild); return NS_OK; -} // ContentRemoved +} NS_IMETHODIMP nsMenuX::ContentInserted(nsIDocument *aDocument, nsIContent *aChild, @@ -1127,7 +1189,7 @@ NS_IMETHODIMP nsMenuX::ContentInserted(nsIDocument *aDocument, nsIContent *aChil SetRebuild(PR_TRUE); return NS_OK; -} // ContentInserted +} NS_IMETHODIMP @@ -1158,12 +1220,12 @@ static pascal OSStatus MyMenuEventHandler(EventHandlerCallRef myHandler, EventRe // this might happen just due to some random quirks in the event system PRUint32 itemCount; nsIMenu* targetMenu = reinterpret_cast(userData); - targetMenu->GetItemCount(itemCount); + targetMenu->GetVisibleItemCount(itemCount); if (aPos >= itemCount) return eventNotHandledErr; - nsISupports* aTargetMenuItem; - targetMenu->GetItemAt((PRUint32)aPos, aTargetMenuItem); + nsCOMPtr aTargetMenuItem; + targetMenu->GetVisibleItemAt((PRUint32)aPos, *getter_AddRefs(aTargetMenuItem)); // Send DOM event // If the QI fails, we're over a submenu and we shouldn't send the event @@ -1216,11 +1278,14 @@ static OSStatus InstallMyMenuEventHandler(MenuRef menuRef, void* userData, Event } +// // MenuDelegate Objective-C class, used to set up Carbon events +// @implementation MenuDelegate + - (id)initWithGeckoMenu:(nsMenuX*)geckoMenu { if ((self = [super init])) { @@ -1230,12 +1295,14 @@ static OSStatus InstallMyMenuEventHandler(MenuRef menuRef, void* userData, Event return self; } + - (void)dealloc { RemoveEventHandler(mEventHandler); [super dealloc]; } + // You can get a MenuRef from an NSMenu*, but not until it has been made visible // or added to the main menu bar. Basically, Cocoa is attempting lazy loading, // and that doesn't work for us. We don't need any carbon events until after the