Improve support for DOM manipulation handling in native menu system, add tests. Re-landing with regression fixes. b=442972 r=kreeger sr=roc
authorJosh Aas <joshmoz@gmail.com>
Mon, 28 Jul 2008 00:46:33 -0400
changeset 16264 1f9c368b25e4d042efe248dc32e7d92d0da9f300
parent 16263 59ed47fea07dc84aa34e3991f063a530b86e2edc
child 16265 b6b0f1c44dc9e1a5d64164a22cd3e065603e1b3c
push id883
push userjosh@mozilla.com
push dateMon, 28 Jul 2008 04:46:58 +0000
treeherdermozilla-central@1f9c368b25e4 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskreeger, roc
bugs442972
milestone1.9.1a2pre
Improve support for DOM manipulation handling in native menu system, add tests. Re-landing with regression fixes. b=442972 r=kreeger sr=roc
dom/public/idl/base/nsIDOMWindowUtils.idl
dom/src/base/nsDOMWindowUtils.cpp
widget/public/nsIWidget.h
widget/src/cocoa/nsChildView.h
widget/src/cocoa/nsChildView.mm
widget/src/cocoa/nsMenuBarX.h
widget/src/cocoa/nsMenuBarX.mm
widget/src/cocoa/nsMenuUtilsX.h
widget/src/cocoa/nsMenuUtilsX.mm
widget/src/cocoa/nsMenuX.h
widget/src/cocoa/nsMenuX.mm
widget/src/xpwidgets/nsBaseWidget.h
widget/tests/native_menus_window.xul
--- a/dom/public/idl/base/nsIDOMWindowUtils.idl
+++ b/dom/public/idl/base/nsIDOMWindowUtils.idl
@@ -165,16 +165,26 @@ interface nsIDOMWindowUtils : nsISupport
    *
    * Cannot be accessed from unprivileged context (not content-accessible)
    * Will throw a DOM security error if called without UniversalXPConnect
    * privileges.
    */
   void activateNativeMenuItemAt(in AString indexString);
 
   /**
+   * See nsIWidget::ForceNativeMenuReload
+   *
+   * This is used for native menu system testing. Calling this forces a full
+   * reload of the menu system, reloading all native menus and their items.
+   * This is important for testing because changes to the DOM can affect the
+   * native menu system lazily.
+   */
+  void forceNativeMenuReload();
+
+  /**
    * Focus the element aElement. The element should be in the same document
    * that the window is displaying. Pass null to blur the element, if any,
    * that currently has focus, and focus the document.
    *
    * Cannot be accessed from unprivileged context (not content-accessible)
    * Will throw a DOM security error if called without UniversalXPConnect
    * privileges.
    *
--- a/dom/src/base/nsDOMWindowUtils.cpp
+++ b/dom/src/base/nsDOMWindowUtils.cpp
@@ -319,16 +319,34 @@ nsDOMWindowUtils::ActivateNativeMenuItem
   // get the widget to send the event to
   nsCOMPtr<nsIWidget> widget = GetWidget();
   if (!widget)
     return NS_ERROR_FAILURE;
 
   return widget->ActivateNativeMenuItemAt(indexString);
 }
 
+
+NS_IMETHODIMP
+nsDOMWindowUtils::ForceNativeMenuReload()
+{
+  PRBool hasCap = PR_FALSE;
+  if (NS_FAILED(nsContentUtils::GetSecurityManager()->IsCapabilityEnabled("UniversalXPConnect", &hasCap))
+      || !hasCap)
+    return NS_ERROR_DOM_SECURITY_ERR;
+
+  // get the widget to send the event to
+  nsCOMPtr<nsIWidget> widget = GetWidget();
+  if (!widget)
+    return NS_ERROR_FAILURE;
+
+  return widget->ForceNativeMenuReload();
+}
+
+
 nsIWidget*
 nsDOMWindowUtils::GetWidget()
 {
   if (mWindow) {
     nsIDocShell *docShell = mWindow->GetDocShell();
     if (docShell) {
       nsCOMPtr<nsIPresShell> presShell;
       docShell->GetPresShell(getter_AddRefs(presShell));
--- a/widget/public/nsIWidget.h
+++ b/widget/public/nsIWidget.h
@@ -1191,16 +1191,24 @@ class nsIWidget : public nsISupports {
      * NS_VK_SCROLL_LOCK.
      * aLEDState is the result for current LED state of the key.
      * If the LED is 'ON', it returns TRUE, otherwise, FALSE.
      * If the platform doesn't support the LED state (or we cannot get the
      * state), this method returns NS_ERROR_NOT_IMPLEMENTED.
      */
     NS_IMETHOD GetToggledKeyState(PRUint32 aKeyCode, PRBool* aLEDState) = 0;
 
+    /**
+     * This is used for native menu system testing. Calling this forces a full
+     * reload of the menu system, reloading all native menus and their items.
+     * This is important for testing because changes to the DOM can affect the
+     * native menu system lazily.
+     */
+    virtual nsresult ForceNativeMenuReload() = 0;
+
 protected:
     // keep the list of children.  We also keep track of our siblings.
     // The ownership model is as follows: parent holds a strong ref to
     // the first element of the list, and each element holds a strong
     // ref to the next element in the list.  The prevsibling and
     // lastchild pointers are weak, which is fine as long as they are
     // maintained properly.
     nsCOMPtr<nsIWidget> mFirstChild;
--- a/widget/src/cocoa/nsChildView.h
+++ b/widget/src/cocoa/nsChildView.h
@@ -326,17 +326,18 @@ public:
   NS_IMETHOD        SetCursor(nsCursor aCursor);
   NS_IMETHOD        SetCursor(imgIContainer* aCursor, PRUint32 aHotspotX, PRUint32 aHotspotY);
   
   NS_IMETHOD        CaptureRollupEvents(nsIRollupListener * aListener, PRBool aDoCapture, PRBool aConsumeRollupEvent);
   NS_IMETHOD        SetTitle(const nsAString& title);
 
   NS_IMETHOD        GetAttention(PRInt32 aCycleCount);
 
-  NS_IMETHOD ActivateNativeMenuItemAt(const nsAString& indexString);
+  NS_IMETHOD        ActivateNativeMenuItemAt(const nsAString& indexString);
+  NS_IMETHOD        ForceNativeMenuReload();
 
   NS_IMETHOD        ResetInputState();
   NS_IMETHOD        SetIMEOpenState(PRBool aState);
   NS_IMETHOD        GetIMEOpenState(PRBool* aState);
   NS_IMETHOD        SetIMEEnabled(PRUint32 aState);
   NS_IMETHOD        GetIMEEnabled(PRUint32* aState);
   NS_IMETHOD        CancelIMEComposition();
   NS_IMETHOD        GetToggledKeyState(PRUint32 aKeyCode,
--- a/widget/src/cocoa/nsChildView.mm
+++ b/widget/src/cocoa/nsChildView.mm
@@ -1375,17 +1375,19 @@ nsresult nsChildView::SynthesizeNativeKe
 
   return NS_OK;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
 }
 
 // Used for testing native menu system structure and event handling.
 NS_IMETHODIMP nsChildView::ActivateNativeMenuItemAt(const nsAString& indexString)
-{  
+{
+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
+
   NSString* title = [NSString stringWithCharacters:indexString.BeginReading() length:indexString.Length()];
   NSArray* indexes = [title componentsSeparatedByString:@"|"];
   unsigned int indexCount = [indexes count];
   if (indexCount == 0)
     return NS_OK;
   
   NSMenu* currentSubmenu = [NSApp mainMenu];
   for (unsigned int i = 0; i < (indexCount - 1); i++) {
@@ -1406,27 +1408,49 @@ NS_IMETHODIMP nsChildView::ActivateNativ
     if (newSubmenu)
       currentSubmenu = newSubmenu;
     else
       return NS_ERROR_FAILURE;
   }
 
   int itemCount = [currentSubmenu numberOfItems];
   int targetIndex = [[indexes objectAtIndex:(indexCount - 1)] intValue];
-  if (targetIndex < itemCount) {
-    // NSLog(@"Performing action for native menu item titled: %@\n",
-    //       [[currentSubmenu itemAtIndex:targetIndex] title]);
-    [currentSubmenu performActionForItemAtIndex:targetIndex];
+  // We can't perform an action on an item with a submenu, that will raise
+  // an obj-c exception.
+  if (targetIndex < itemCount && ![[currentSubmenu itemAtIndex:targetIndex] hasSubmenu]) {
+      // NSLog(@"Performing action for native menu item titled: %@\n",
+      //       [[currentSubmenu itemAtIndex:targetIndex] title]);
+      [currentSubmenu performActionForItemAtIndex:targetIndex];      
   }
   else {
     return NS_ERROR_FAILURE;
   }
 
   return NS_OK;
-}
+
+  NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
+}
+
+
+NS_IMETHODIMP nsChildView::ForceNativeMenuReload()
+{
+  id windowDelegate = [[mView nativeWindow] delegate];
+  if (windowDelegate && [windowDelegate isKindOfClass:[WindowDelegate class]]) {
+    nsCocoaWindow *widget = [(WindowDelegate *)windowDelegate geckoWidget];
+    if (widget) {
+      nsMenuBarX* mb = widget->GetMenuBar();
+      if (mb) {
+        mb->ForceNativeMenuReload();
+      }
+    }
+  }
+
+  return NS_OK;
+}
+
 
 #pragma mark -
 
 
 #ifdef INVALIDATE_DEBUGGING
 
 static Boolean KeyDown(const UInt8 theKey)
 {
--- a/widget/src/cocoa/nsMenuBarX.h
+++ b/widget/src/cocoa/nsMenuBarX.h
@@ -101,42 +101,45 @@ public:
   nsCOMPtr<nsIContent> mAboutItemContent;
   nsCOMPtr<nsIContent> mPrefItemContent;
   nsCOMPtr<nsIContent> mQuitItemContent;
 
   NS_DECL_ISUPPORTS
   NS_DECL_NSIMUTATIONOBSERVER
 
   // nsMenuObjectX
-  void*             NativeData()     {return (void*)mRootMenu;}
+  void*             NativeData()     {return (void*)mNativeMenu;}
   nsMenuObjectTypeX MenuObjectType() {return eMenuBarObjectType;}
 
   // nsMenuBarX
   nsresult          Create(nsIWidget* aParent, nsIContent* aContent);
   void              SetParent(nsIWidget* aParent);
   void              RegisterForContentChanges(nsIContent* aContent, nsChangeObserver* aMenuObject);
   void              UnregisterForContentChanges(nsIContent* aContent);
   PRUint32          RegisterForCommand(nsMenuItemX* aItem);
   void              UnregisterCommand(PRUint32 aCommandID);
   PRUint32          GetMenuCount();
+  bool              MenuContainsAppMenu();
   nsMenuX*          GetMenuAt(PRUint32 aIndex);
   nsMenuItemX*      GetMenuItemForCommandID(PRUint32 inCommandID);
   nsresult          Paint();
+  void              ForceNativeMenuReload(); // used for testing
 
 protected:
-  nsresult          AddMenu(nsMenuX* aMenu);
-  void              RemoveMenu(PRUint32 aIndex);
+  void              ConstructNativeMenus();
+  nsresult          InsertMenuAtIndex(nsMenuX* aMenu, PRUint32 aIndex);
+  void              RemoveMenuAtIndex(PRUint32 aIndex);
   nsChangeObserver* LookupContentChangeObserver(nsIContent* aContent);
   void              HideItem(nsIDOMDocument* inDoc, const nsAString & inID, nsIContent** outHiddenNode);
   void              AquifyMenuBar();
   NSMenuItem*       CreateNativeAppMenuItem(nsMenuX* inMenu, const nsAString& nodeID, SEL action,
                                             int tag, NativeMenuItemTarget* target);
   nsresult          CreateApplicationMenu(nsMenuX* inMenu);
 
   nsTArray< nsAutoPtr<nsMenuX> > mMenuArray;
-  nsIWidget*         mParent;              // [weak]
+  nsIWidget*         mParentWindow;        // [weak]
   PRUint32           mCurrentCommandID;    // unique command id (per menu-bar) to give to next item that asks
   nsIDocument*       mDocument;            // pointer to document
-  GeckoNSMenu*       mRootMenu;            // root menu, representing entire menu bar
+  GeckoNSMenu*       mNativeMenu;            // root menu, representing entire menu bar
   nsHashtable        mObserverTable;       // stores observers for content change notification
 };
 
 #endif // nsMenuBarX_h_
--- a/widget/src/cocoa/nsMenuBarX.mm
+++ b/widget/src/cocoa/nsMenuBarX.mm
@@ -93,23 +93,23 @@ NS_IMETHODIMP nsNativeMenuServiceX::Crea
   if (!mb)
     return NS_ERROR_OUT_OF_MEMORY;
 
   return mb->Create(aParent, aMenuBarNode);
 }
 
 
 nsMenuBarX::nsMenuBarX()
-: mParent(nsnull),
+: mParentWindow(nsnull),
   mCurrentCommandID(eCommand_ID_Last),
   mDocument(nsnull)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
-  mRootMenu = [[GeckoNSMenu alloc] initWithTitle:@"MainMenuBar"];
+  mNativeMenu = [[GeckoNSMenu alloc] initWithTitle:@"MainMenuBar"];
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 
 nsMenuBarX::~nsMenuBarX()
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
@@ -131,145 +131,187 @@ nsMenuBarX::~nsMenuBarX()
     mDocument->RemoveMutationObserver(this);
 
   // We have to manually clear the array here because clearing causes menu items
   // to call back into the menu bar to unregister themselves. We don't want to
   // depend on member variable ordering to ensure that the array gets cleared
   // before the registration hash table is destroyed.
   mMenuArray.Clear();
 
-  [mRootMenu release];
+  [mNativeMenu release];
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 
 nsresult nsMenuBarX::Create(nsIWidget* aParent, nsIContent* aContent)
 {
   if (!aParent || !aContent)
     return NS_ERROR_INVALID_ARG;
 
-  mParent = aParent;
+  mParentWindow = aParent;
   mContent = aContent;
 
   AquifyMenuBar();
 
   nsIDocument* doc = aContent->GetOwnerDoc();
   if (!doc)
     return NS_ERROR_FAILURE;
   doc->AddMutationObserver(this);
   mDocument = doc;
 
+  ConstructNativeMenus();
+
+  // Give this to the parent window. The parent takes ownership.
+  return mParentWindow->SetMenuBar(this);
+}
+
+
+void nsMenuBarX::ConstructNativeMenus()
+{
   PRUint32 count = mContent->GetChildCount();
   for (PRUint32 i = 0; i < count; i++) { 
     nsIContent *menuContent = mContent->GetChildAt(i);
-    if (menuContent) {
-      if (menuContent->Tag() == nsWidgetAtoms::menu &&
-          menuContent->IsNodeOfType(nsINode::eXUL)) {
-        nsAutoString menuName;
-        menuContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::label, menuName);
-        nsMenuX* newMenu = new nsMenuX();
-        if (newMenu) {
-          nsresult rv = newMenu->Create(this, menuName, this, menuContent);
-          if (NS_SUCCEEDED(rv))
-            AddMenu(newMenu);
-          else
-            delete newMenu;
-        }
+    if (menuContent &&
+        menuContent->Tag() == nsWidgetAtoms::menu &&
+        menuContent->IsNodeOfType(nsINode::eXUL)) {
+      nsMenuX* newMenu = new nsMenuX();
+      if (newMenu) {
+        nsresult rv = newMenu->Create(this, this, menuContent);
+        if (NS_SUCCEEDED(rv))
+          InsertMenuAtIndex(newMenu, GetMenuCount());
+        else
+          delete newMenu;
       }
     }
-  }
-
-  // Give this to the parent window. The parent takes ownership.
-  return mParent->SetMenuBar(this);
+  }  
 }
 
 
 PRUint32 nsMenuBarX::GetMenuCount()
 {
   return mMenuArray.Length();
 }
 
 
-nsresult nsMenuBarX::AddMenu(nsMenuX* aMenu)
+bool nsMenuBarX::MenuContainsAppMenu()
+{
+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
+
+  return ([mNativeMenu numberOfItems] > 0 &&
+          [[mNativeMenu itemAtIndex:0] submenu] == sApplicationMenu);
+
+  NS_OBJC_END_TRY_ABORT_BLOCK_RETURN(false);
+}
+
+
+nsresult nsMenuBarX::InsertMenuAtIndex(nsMenuX* aMenu, PRUint32 aIndex)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
 
   // If we haven't created a global Application menu yet, do it.
   if (!sApplicationMenu) {
     nsresult rv = NS_OK; // avoid warning about rv being unused
     rv = CreateApplicationMenu(aMenu);
     NS_ASSERTION(NS_SUCCEEDED(rv), "Can't create Application menu");
 
     // Hook the new Application menu up to the menu bar.
     NSMenu* mainMenu = [NSApp mainMenu];
     NS_ASSERTION([mainMenu numberOfItems] > 0, "Main menu does not have any items, something is terribly wrong!");
     [[mainMenu itemAtIndex:0] setSubmenu:sApplicationMenu];
   }
 
-  // keep track of all added menus
-  mMenuArray.AppendElement(aMenu); // owner
+  // add menu to array that owns our menus
+  mMenuArray.InsertElementAt(aIndex, aMenu);
 
-  NSMenu* nativeMenu = (NSMenu*)aMenu->NativeData();
+  // hook up submenus
   nsIContent* menuContent = aMenu->Content();
   if (menuContent->GetChildCount() > 0 &&
       !nsMenuUtilsX::NodeIsHiddenOrCollapsed(menuContent)) {
-    NSMenuItem* newMenuItem = [[[NSMenuItem alloc] initWithTitle:[nativeMenu title] action:NULL keyEquivalent:@""] autorelease];
-    [mRootMenu addItem:newMenuItem];
-    [newMenuItem setSubmenu:nativeMenu];
+    PRUint32 insertAfter = 0;
+    nsresult rv = nsMenuUtilsX::CountVisibleBefore(this, aMenu, &insertAfter);
+    NS_ASSERTION(NS_SUCCEEDED(rv), "nsMenuUtilsX::CountVisibleBefore failed!\n");
+    if (NS_FAILED(rv))
+      return rv;
+    if (MenuContainsAppMenu())
+      insertAfter++;
+    [mNativeMenu insertItem:aMenu->NativeMenuItem() atIndex:insertAfter];
   }
 
   return NS_OK;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
 }
 
 
-void nsMenuBarX::RemoveMenu(PRUint32 aIndex)
+void nsMenuBarX::RemoveMenuAtIndex(PRUint32 aIndex)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   NS_ASSERTION(aIndex < mMenuArray.Length(), "Attempting submenu removal with bad index!");
 
   // Our native menu and our internal menu object array might be out of sync.
   // This happens, for example, when a submenu is hidden. Because of this we
   // should not assume that a native submenu is hooked up.
-  [mRootMenu removeItem:(mMenuArray[aIndex]->NativeMenuItem())];
+  NSMenuItem* nativeMenuItem = mMenuArray[aIndex]->NativeMenuItem();
+  int nativeMenuItemIndex = [mNativeMenu indexOfItem:nativeMenuItem];
+  if (nativeMenuItemIndex != -1)
+    [mNativeMenu removeItemAtIndex:nativeMenuItemIndex];
+
   mMenuArray.RemoveElementAt(aIndex);
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 
+// Calling this forces a full reload of the menu system, reloading all native
+// menus and their items.
+// Without this testing is hard because changes to the DOM affect the native
+// menu system lazily.
+void nsMenuBarX::ForceNativeMenuReload()
+{
+  // tear down everything
+  while (GetMenuCount() > 0)
+    RemoveMenuAtIndex(0);
+
+  // construct everything
+  ConstructNativeMenus();
+}
+
+
 nsMenuX* nsMenuBarX::GetMenuAt(PRUint32 aIndex)
 {
   if (mMenuArray.Length() <= aIndex) {
     NS_ERROR("Requesting menu at invalid index!");
     return NULL;
   }
   return mMenuArray[aIndex];
 }
 
 
 nsresult nsMenuBarX::Paint()
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
-  
-  NSMenu* mainMenu = [NSApp mainMenu];
-  NS_ASSERTION([mainMenu numberOfItems] > 0, "Main menu does not have any items, something is terribly wrong!");
+
+  // Don't try to optimize anything in this painting by checking
+  // sLastGeckoMenuBarPainted because the menubar can be manipulated by
+  // native dialogs and sheet code and other things besides this paint method.
 
-  // Swap out first item into incoming menu bar. We have to keep the same menu item for the
-  // Application menu and its submenu is global so we keep passing it along.
-  NSMenuItem* firstMenuItem = [[mainMenu itemAtIndex:0] retain];
-  [mainMenu removeItemAtIndex:0];
-  [mRootMenu insertItem:firstMenuItem atIndex:0];
-  [firstMenuItem release];
+  // We have to keep the same menu item for the Application menu so we keep
+  // passing it along.
+  NSMenu* outgoingMenu = [NSApp mainMenu];
+  NS_ASSERTION([outgoingMenu numberOfItems] > 0, "Main menu does not have any items, something is terribly wrong!");
+
+  NSMenuItem* appMenuItem = [[outgoingMenu itemAtIndex:0] retain];
+  [outgoingMenu removeItemAtIndex:0];
+  [mNativeMenu insertItem:appMenuItem atIndex:0];
+  [appMenuItem release];
 
   // Set menu bar and event target.
-  [NSApp setMainMenu:mRootMenu];
+  [NSApp setMainMenu:mNativeMenu];
   nsMenuBarX::sLastGeckoMenuBarPainted = this;
 
   gSomeMenuBarPainted = YES;
 
   return NS_OK;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
 }
@@ -549,23 +591,25 @@ nsresult nsMenuBarX::CreateApplicationMe
   return (sApplicationMenu) ? NS_OK : NS_ERROR_FAILURE;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
 }
 
 
 void nsMenuBarX::SetParent(nsIWidget* aParent)
 {
-  mParent = aParent;
+  mParentWindow = aParent;
 }
 
+
 //
 // nsIMutationObserver
 //
 
+
 void nsMenuBarX::CharacterDataWillChange(nsIDocument* aDocument,
                                          nsIContent* aContent,
                                          CharacterDataChangeInfo* aInfo)
 {
 }
 
 
 void nsMenuBarX::CharacterDataChanged(nsIDocument* aDocument,
@@ -573,81 +617,89 @@ void nsMenuBarX::CharacterDataChanged(ns
                                       CharacterDataChangeInfo* aInfo)
 {
 }
 
 
 void nsMenuBarX::ContentAppended(nsIDocument* aDocument, nsIContent* aContainer,
                                  PRInt32 aNewIndexInContainer)
 {
-  if (aContainer != mContent) {
-    nsChangeObserver* obs = LookupContentChangeObserver(aContainer);
-    if (obs)
-      obs->ObserveContentInserted(aDocument, aContainer, aNewIndexInContainer);
-    else {
-      nsCOMPtr<nsIContent> parent = aContainer->GetParent();
-      if (parent) {
-        obs = LookupContentChangeObserver(parent);
-        if (obs)
-          obs->ObserveContentInserted(aDocument, aContainer, aNewIndexInContainer);
-      }
-    }
+  PRUint32 childCount = aContainer->GetChildCount();
+  while ((PRUint32)aNewIndexInContainer < childCount) {
+    nsIContent *child = aContainer->GetChildAt(aNewIndexInContainer);
+    ContentInserted(aDocument, aContainer, child, aNewIndexInContainer);
+    aNewIndexInContainer++;
   }
 }
 
 
 void nsMenuBarX::NodeWillBeDestroyed(const nsINode * aNode)
 {
+  // our menu bar node is being destroyed
   mDocument = nsnull;
 }
 
 
 void nsMenuBarX::AttributeChanged(nsIDocument * aDocument, nsIContent * aContent,
                                   PRInt32 aNameSpaceID, nsIAtom * aAttribute,
                                   PRInt32 aModType, PRUint32 aStateMask)
 {
-  // lookup and dispatch to registered thang
   nsChangeObserver* obs = LookupContentChangeObserver(aContent);
   if (obs)
     obs->ObserveAttributeChanged(aDocument, aContent, aAttribute);
 }
 
 
 void nsMenuBarX::ContentRemoved(nsIDocument * aDocument, nsIContent * aContainer,
                                 nsIContent * aChild, PRInt32 aIndexInContainer)
 {
   if (aContainer == mContent) {
-    UnregisterForContentChanges(aChild);
-    RemoveMenu(aIndexInContainer);
+    RemoveMenuAtIndex(aIndexInContainer);
   }
   else {
     nsChangeObserver* obs = LookupContentChangeObserver(aContainer);
     if (obs) {
       obs->ObserveContentRemoved(aDocument, aChild, aIndexInContainer);
     }
     else {
+      // We do a lookup on the parent container in case things were removed
+      // under a "menupopup" item. That is basically a wrapper for the contents
+      // of a "menu" node.
       nsCOMPtr<nsIContent> parent = aContainer->GetParent();
       if (parent) {
         obs = LookupContentChangeObserver(parent);
         if (obs)
           obs->ObserveContentRemoved(aDocument, aChild, aIndexInContainer);
       }
     }
   }
 }
 
 
 void nsMenuBarX::ContentInserted(nsIDocument * aDocument, nsIContent * aContainer,
                                  nsIContent * aChild, PRInt32 aIndexInContainer)
 {
-  if (aContainer != mContent) {
+  if (aContainer == mContent) {
+    nsMenuX* newMenu = new nsMenuX();
+    if (newMenu) {
+      nsresult rv = newMenu->Create(this, this, aChild);
+      if (NS_SUCCEEDED(rv))
+        InsertMenuAtIndex(newMenu, aIndexInContainer);
+      else
+        delete newMenu;
+    }
+  }
+  else {
     nsChangeObserver* obs = LookupContentChangeObserver(aContainer);
     if (obs)
       obs->ObserveContentInserted(aDocument, aChild, aIndexInContainer);
     else {
+      // We do a lookup on the parent container in case things were removed
+      // under a "menupopup" item. That is basically a wrapper for the contents
+      // of a "menu" node.
       nsCOMPtr<nsIContent> parent = aContainer->GetParent();
       if (parent) {
         obs = LookupContentChangeObserver(parent);
         if (obs)
           obs->ObserveContentInserted(aDocument, aChild, aIndexInContainer);
       }
     }
   }
--- a/widget/src/cocoa/nsMenuUtilsX.h
+++ b/widget/src/cocoa/nsMenuUtilsX.h
@@ -36,16 +36,17 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
 #ifndef nsMenuUtilsX_h_
 #define nsMenuUtilsX_h_
 
 #include "nscore.h"
 #include "nsGUIEvent.h"
+#include "nsMenuBaseX.h"
 
 #import <Cocoa/Cocoa.h>
 
 class nsIContent;
 class nsString;
 class nsMenuBarX;
 
 // Namespace containing utility functions used in our native menu implementation.
@@ -53,11 +54,12 @@ namespace nsMenuUtilsX
 {
   nsEventStatus DispatchCommandTo(nsIContent* aTargetContent);
   NSString*     CreateTruncatedCocoaLabel(const nsString& itemLabel); // returned object is not retained
   PRUint8       GeckoModifiersForNodeAttribute(const nsString& modifiersAttribute);
   unsigned int  MacModifiersForGeckoModifiers(PRUint8 geckoModifiers);
   nsMenuBarX*   GetHiddenWindowMenuBar(); // returned object is not retained
   NSMenuItem*   GetStandardEditMenuItem(); // returned object is not retained
   PRBool        NodeIsHiddenOrCollapsed(nsIContent* inContent);
+  nsresult      CountVisibleBefore(nsMenuObjectX* aMenuObject, nsMenuObjectX* aChild, PRUint32* outVisibleBefore);
 }
 
 #endif // nsMenuUtilsX_h_
--- a/widget/src/cocoa/nsMenuUtilsX.mm
+++ b/widget/src/cocoa/nsMenuUtilsX.mm
@@ -33,16 +33,17 @@
  * and other provisions required by the GPL or the LGPL. If you do not delete
  * the provisions above, a recipient may use your version of this file under
  * the terms of any one of the MPL, the GPL or the LGPL.
  *
  * ***** END LICENSE BLOCK ***** */
 
 #include "nsMenuUtilsX.h"
 #include "nsMenuBarX.h"
+#include "nsMenuX.h"
 #include "nsMenuItemX.h"
 #include "nsObjCExceptions.h"
 #include "nsCocoaUtils.h"
 #include "nsCocoaWindow.h"
 #include "nsWidgetAtoms.h"
 
 #import <Carbon/Carbon.h>
 
@@ -190,8 +191,49 @@ NSMenuItem* nsMenuUtilsX::GetStandardEdi
 
 PRBool nsMenuUtilsX::NodeIsHiddenOrCollapsed(nsIContent* inContent)
 {
   return (inContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::hidden,
                                  nsWidgetAtoms::_true, eCaseMatters) ||
           inContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::collapsed,
                                  nsWidgetAtoms::_true, eCaseMatters));
 }
+
+
+// Determines how many items are visible among the siblings in a menu that are
+// before the given child. Note that this will not count the application menu.
+nsresult nsMenuUtilsX::CountVisibleBefore(nsMenuObjectX* aParentMenu, nsMenuObjectX* aChild, PRUint32* outVisibleBefore)
+{
+  NS_ASSERTION(outVisibleBefore, "bad index param in nsMenuX::CountVisibleBefore");
+
+  nsMenuObjectTypeX parentType = aParentMenu->MenuObjectType();
+  if (parentType == eMenuBarObjectType) {
+    *outVisibleBefore = 0;
+    nsMenuBarX* menubarParent = static_cast<nsMenuBarX*>(aParentMenu);
+    PRUint32 numMenus = menubarParent->GetMenuCount();
+    for (PRUint32 i = 0; i < numMenus; i++) {
+      nsMenuX* currMenu = menubarParent->GetMenuAt(i);
+      if (currMenu == aChild)
+        return NS_OK; // we found ourselves, break out
+      if (currMenu) {
+        nsIContent* menuContent = currMenu->Content();
+        if (menuContent->GetChildCount() > 0 &&
+            !nsMenuUtilsX::NodeIsHiddenOrCollapsed(menuContent)) {
+          ++(*outVisibleBefore);
+        }
+      }
+    }
+  }
+  else if (parentType == eSubmenuObjectType) {
+    *outVisibleBefore = 0;
+    nsMenuX* menuParent = static_cast<nsMenuX*>(aParentMenu);
+    PRUint32 numItems = menuParent->GetItemCount();
+    for (PRUint32 i = 0; i < numItems; i++) {
+      // Using GetItemAt instead of GetVisibleItemAt to avoid O(N^2)
+      nsMenuObjectX* currItem = menuParent->GetItemAt(i);
+      if (currItem == aChild)
+        return NS_OK; // we found ourselves, break out
+      if (!nsMenuUtilsX::NodeIsHiddenOrCollapsed(currItem->Content()))
+        ++(*outVisibleBefore);
+    }
+  }
+  return NS_ERROR_FAILURE;
+}
--- a/widget/src/cocoa/nsMenuX.h
+++ b/widget/src/cocoa/nsMenuX.h
@@ -78,38 +78,36 @@ public:
   // If > 0, the OS is indexing all the app's menus (triggered by opening
   // Help menu on Leopard and higher).  There are some things that are
   // unsafe to do while this is happening.
   static PRInt32 sIndexingMenuLevel;
 
   NS_DECL_CHANGEOBSERVER
 
   // nsMenuObjectX
-  void*             NativeData()     {return (void*)mMacMenu;}
+  void*             NativeData()     {return (void*)mNativeMenu;}
   nsMenuObjectTypeX MenuObjectType() {return eSubmenuObjectType;}
 
   // nsMenuX
-  nsresult       Create(nsMenuObjectX* aParent, const nsAString &aLabel,
-                        nsMenuBarX* aMenuBar, nsIContent* aNode);
+  nsresult       Create(nsMenuObjectX* aParent, nsMenuBarX* aMenuBar, nsIContent* aNode);
   PRUint32       GetItemCount();
   nsMenuObjectX* GetItemAt(PRUint32 aPos);
   nsresult       GetVisibleItemCount(PRUint32 &aCount);
   nsMenuObjectX* GetVisibleItemAt(PRUint32 aPos);
   nsEventStatus  MenuOpened(const nsMenuEvent& aMenuEvent);
   void           MenuClosed(const nsMenuEvent& aMenuEvent);
   void           SetRebuild(PRBool aMenuEvent);
   NSMenuItem*    NativeMenuItem();
 
 protected:
-  void           MenuConstruct(nsIWidget* aParentWindow, void* aMenuNode);
+  void           MenuConstruct();
   nsresult       RemoveAll();
   nsresult       SetEnabled(PRBool aIsEnabled);
   nsresult       GetEnabled(PRBool* aIsEnabled);
   nsresult       SetupIcon();
-  nsresult       CountVisibleBefore(PRUint32* outVisibleBefore);
   void           GetMenuPopupContent(nsIContent** aResult);
   PRBool         OnOpen();
   PRBool         OnOpened();
   PRBool         OnClose();
   PRBool         OnClosed();
   nsresult       AddMenuItem(nsMenuItemX* aMenuItem);
   nsresult       AddMenu(nsMenuX* aMenu);
   void           LoadMenuItem(nsIContent* inMenuItemContent);  
@@ -117,17 +115,17 @@ protected:
   GeckoNSMenu*   CreateMenuWithGeckoString(nsString& menuTitle);
 
   nsTArray< nsAutoPtr<nsMenuObjectX> > mMenuObjectsArray;
   nsString                  mLabel;
   PRUint32                  mVisibleItemsCount; // cache
   nsMenuObjectX*            mParent; // [weak]
   nsMenuBarX*               mMenuBar; // [weak]
   nsRefPtr<nsMenuItemIconX> mIcon;
-  GeckoNSMenu*              mMacMenu; // [strong]
+  GeckoNSMenu*              mNativeMenu; // [strong]
   MenuDelegate*             mMenuDelegate; // [strong]
   NSMenuItem*               mNativeMenuItem; // [strong]
   PRPackedBool              mIsEnabled;
   PRPackedBool              mDestroyHandlerCalled;
   PRPackedBool              mNeedsRebuild;
   PRPackedBool              mConstructed;
   PRPackedBool              mVisible;
   PRPackedBool              mXBLAttached;
--- a/widget/src/cocoa/nsMenuX.mm
+++ b/widget/src/cocoa/nsMenuX.mm
@@ -79,17 +79,17 @@ extern "C" MenuRef _NSGetCarbonMenu(NSMe
 static PRBool gConstructingMenu = PR_FALSE;
 static PRBool gMenuMethodsSwizzled = PR_FALSE;
 
 PRInt32 nsMenuX::sIndexingMenuLevel = 0;
 
 
 nsMenuX::nsMenuX()
 : mVisibleItemsCount(0), mParent(nsnull), mMenuBar(nsnull),
-  mMacMenu(nil), mNativeMenuItem(nil), mIsEnabled(PR_TRUE),
+  mNativeMenu(nil), mNativeMenuItem(nil), mIsEnabled(PR_TRUE),
   mDestroyHandlerCalled(PR_FALSE), mNeedsRebuild(PR_TRUE),
   mConstructed(PR_FALSE), mVisible(PR_TRUE), mXBLAttached(PR_FALSE)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   if (nsToolkit::OnLeopardOrLater() && !gMenuMethodsSwizzled) {
     nsToolkit::SwizzleMethods([NSMenu class], @selector(_addItem:toTable:),
                               @selector(nsMenuX_NSMenu_addItem:toTable:), PR_TRUE);
@@ -113,39 +113,38 @@ nsMenuX::nsMenuX()
 
 
 nsMenuX::~nsMenuX()
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
 
   RemoveAll();
 
-  [mMacMenu setDelegate:nil];
-  [mMacMenu release];
+  [mNativeMenu setDelegate:nil];
+  [mNativeMenu release];
   [mMenuDelegate release];
   [mNativeMenuItem release];
 
   // alert the change notifier we don't care no more
   if (mContent)
     mMenuBar->UnregisterForContentChanges(mContent);
 
   MOZ_COUNT_DTOR(nsMenuX);
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 
-nsresult nsMenuX::Create(nsMenuObjectX* aParent, const nsAString& aLabel,
-                         nsMenuBarX* aMenuBar, nsIContent* aNode)
+nsresult nsMenuX::Create(nsMenuObjectX* aParent, nsMenuBarX* aMenuBar, nsIContent* aNode)
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
 
   mContent = aNode;
-  mLabel = aLabel;
-  mMacMenu = CreateMenuWithGeckoString(mLabel);
+  mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::label, mLabel);
+  mNativeMenu = CreateMenuWithGeckoString(mLabel);
 
   // register this menu to be notified when changes are made to our content object
   mMenuBar = aMenuBar; // weak ref
   NS_ASSERTION(mMenuBar, "No menu bar given, must have one");
   mMenuBar->RegisterForContentChanges(mContent, this);
 
   mParent = aParent;
   // our parent could be either a menu bar (if we're toplevel) or a menu (if we're a submenu)
@@ -159,24 +158,25 @@ nsresult nsMenuX::Create(nsMenuObjectX* 
     mVisible = PR_FALSE;
 
   SetEnabled(!mContent->AttrValueIs(kNameSpaceID_None, nsWidgetAtoms::disabled,
                                     nsWidgetAtoms::_true, eCaseMatters));
 
   NSString *newCocoaLabelString = nsMenuUtilsX::CreateTruncatedCocoaLabel(mLabel);
   mNativeMenuItem = [[NSMenuItem alloc] initWithTitle:newCocoaLabelString action:nil keyEquivalent:@""];
   [newCocoaLabelString release];
+  [mNativeMenuItem setSubmenu:mNativeMenu];
 
   [mNativeMenuItem setEnabled:(BOOL)mIsEnabled];
 
   // We call MenuConstruct here because keyboard commands are dependent upon
   // native menu items being created. If we only call MenuConstruct when a menu
   // is actually selected, then we can't access keyboard commands until the
   // menu gets selected, which is bad.
-  MenuConstruct(nsnull, nsnull);
+  MenuConstruct();
 
   mIcon = new nsMenuItemIconX(this, mContent, mNativeMenuItem);
 
   return NS_OK;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
 }
 
@@ -191,17 +191,17 @@ nsresult nsMenuX::AddMenuItem(nsMenuItem
   mMenuObjectsArray.AppendElement(aMenuItem);
   if (nsMenuUtilsX::NodeIsHiddenOrCollapsed(aMenuItem->Content()))
     return NS_OK;
   ++mVisibleItemsCount;
 
   NSMenuItem* newNativeMenuItem = (NSMenuItem*)aMenuItem->NativeData();
 
   // add the menu item to this menu
-  [mMacMenu addItem:newNativeMenuItem];
+  [mNativeMenu addItem:newNativeMenuItem];
 
   // set up target/action
   [newNativeMenuItem setTarget:nsMenuBarX::sNativeEventTarget];
   [newNativeMenuItem setAction:@selector(menuItemHit:)];
 
   // set its command. we get the unique command id from the menubar
   [newNativeMenuItem setTag:mMenuBar->RegisterForCommand(aMenuItem)];
 
@@ -220,20 +220,20 @@ nsresult nsMenuX::AddMenu(nsMenuX* aMenu
     return NS_ERROR_NULL_POINTER;
 
   mMenuObjectsArray.AppendElement(aMenu);
   if (nsMenuUtilsX::NodeIsHiddenOrCollapsed(aMenu->Content()))
     return NS_OK;
   ++mVisibleItemsCount;
 
   // We have to add a menu item and then associate the menu with it
-  NSMenuItem* newNativeMenuItem = (static_cast<nsMenuX*>(aMenu))->NativeMenuItem();
+  NSMenuItem* newNativeMenuItem = aMenu->NativeMenuItem();
   if (!newNativeMenuItem)
     return NS_ERROR_FAILURE;
-  [mMacMenu addItem:newNativeMenuItem];
+  [mNativeMenu addItem:newNativeMenuItem];
 
   [newNativeMenuItem setSubmenu:(NSMenu*)aMenu->NativeData()];
 
   return NS_OK;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
 }
 
@@ -250,24 +250,16 @@ nsMenuObjectX* nsMenuX::GetItemAt(PRUint
 {
   if (aPos >= (PRUint32)mMenuObjectsArray.Length())
     return NULL;
 
   return mMenuObjectsArray[aPos];
 }
 
 
-// Checks submenus and menu items. Not suitable for submenus that are children
-// of a menu bar, which has slightly different rules for visiblity.
-static PRBool MenuNodeIsVisible(nsMenuObjectX* item)
-{
-  return (!nsMenuUtilsX::NodeIsHiddenOrCollapsed(item->Content()));
-}
-
-
 // Only includes visible items
 nsresult nsMenuX::GetVisibleItemCount(PRUint32 &aCount)
 {
   aCount = mVisibleItemsCount;
   return NS_OK;
 }
 
 
@@ -284,41 +276,41 @@ nsMenuObjectX* nsMenuX::GetVisibleItemAt
   if (mVisibleItemsCount == count)
     return mMenuObjectsArray[aPos];
 
   // Otherwise, traverse the array until we find the the item we're looking for.
   nsMenuObjectX* item;
   PRUint32 visibleNodeIndex = 0;
   for (PRUint32 i = 0; i < count; i++) {
     item = mMenuObjectsArray[i];
-    if (MenuNodeIsVisible(item)) {
+    if (!nsMenuUtilsX::NodeIsHiddenOrCollapsed(item->Content())) {
       if (aPos == visibleNodeIndex) {
         // we found the visible node we're looking for, return it
         return item;
       }
       visibleNodeIndex++;
     }
   }
 
   return NULL;
 }
 
 
 nsresult nsMenuX::RemoveAll()
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_NSRESULT;
 
-  if (mMacMenu) {
+  if (mNativeMenu) {
     // clear command id's
-    int itemCount = [mMacMenu numberOfItems];
+    int itemCount = [mNativeMenu numberOfItems];
     for (int i = 0; i < itemCount; i++)
-      mMenuBar->UnregisterCommand((PRUint32)[[mMacMenu itemAtIndex:i] tag]);
+      mMenuBar->UnregisterCommand((PRUint32)[[mNativeMenu itemAtIndex:i] tag]);
     // get rid of Cocoa menu items
-    for (int i = [mMacMenu numberOfItems] - 1; i >= 0; i--)
-      [mMacMenu removeItemAtIndex:i];
+    for (int i = [mNativeMenu numberOfItems] - 1; i >= 0; i--)
+      [mNativeMenu removeItemAtIndex:i];
   }
 
   mMenuObjectsArray.Clear();
   mVisibleItemsCount = 0;
 
   return NS_OK;
 
   NS_OBJC_END_TRY_ABORT_BLOCK_NSRESULT;
@@ -329,31 +321,31 @@ nsEventStatus nsMenuX::MenuOpened(const 
 {
   NS_OBJC_BEGIN_TRY_ABORT_BLOCK_RETURN;
 
   // Determine if this is the correct menu to handle the event
   MenuRef selectedMenuHandle = (MenuRef)aMenuEvent.mCommand;
 
   // at this point, the carbon event handler was installed so there
   // must be a carbon MenuRef to be had
-  if (_NSGetCarbonMenu(mMacMenu) == selectedMenuHandle) {
+  if (_NSGetCarbonMenu(mNativeMenu) == selectedMenuHandle) {
     // Open the node.
     mContent->SetAttr(kNameSpaceID_None, nsWidgetAtoms::open, NS_LITERAL_STRING("true"), PR_TRUE);
 
     // Fire a handler. If we're told to stop, don't build the menu at all
     PRBool keepProcessing = OnOpen();
 
     if (!mNeedsRebuild || !keepProcessing)
       return nsEventStatus_eConsumeNoDefault;
 
     if (!mConstructed || mNeedsRebuild) {
       if (mNeedsRebuild)
         RemoveAll();
 
-      MenuConstruct(nsnull, nsnull);
+      MenuConstruct();
       mConstructed = true;
     }
 
     OnOpened();
 
     return nsEventStatus_eConsumeNoDefault;  
   }
   else {
@@ -389,26 +381,26 @@ void nsMenuX::MenuClosed(const nsMenuEve
 
     OnClosed();
 
     mConstructed = false;
   }
 }
 
 
-void nsMenuX::MenuConstruct(nsIWidget* aParentWindow, void* aMenuNode)
+void nsMenuX::MenuConstruct()
 {
   mConstructed = false;
   gConstructingMenu = PR_TRUE;
   
   // reset destroy handler flag so that we'll know to fire it next time this menu goes away.
   mDestroyHandlerCalled = PR_FALSE;
-  
-  //printf("nsMenuX::MenuConstruct called for %s = %d \n", NS_LossyConvertUTF16toASCII(mLabel).get(), mMacMenu);
-  
+
+  //printf("nsMenuX::MenuConstruct called for %s = %d \n", NS_LossyConvertUTF16toASCII(mLabel).get(), mNativeMenu);
+
   // Retrieve our menupopup.
   nsCOMPtr<nsIContent> menuPopup;
   GetMenuPopupContent(getter_AddRefs(menuPopup));
   if (!menuPopup) {
     gConstructingMenu = PR_FALSE;
     return;
   }
 
@@ -546,25 +538,21 @@ void nsMenuX::LoadMenuItem(nsIContent* i
   // This needs to happen after the nsIMenuItem object is inserted into
   // our item array in AddMenuItem()
   menuItem->SetupIcon();
 }
 
 
 void nsMenuX::LoadSubMenu(nsIContent* inMenuContent)
 {
-  nsAutoString menuName; 
-  inMenuContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::label, menuName);
-  //printf("Creating Menu [%s] \n", NS_LossyConvertUTF16toASCII(menuName).get());
-
   nsAutoPtr<nsMenuX> menu(new nsMenuX());
   if (!menu)
     return;
 
-  nsresult rv = menu->Create(this, menuName, mMenuBar, inMenuContent);
+  nsresult rv = menu->Create(this, mMenuBar, inMenuContent);
   if (NS_FAILED(rv))
     return;
 
   AddMenu(menu);
 
   // This needs to happen after the nsIMenu object is inserted into
   // our item array in AddMenu()
   menu->SetupIcon();
@@ -749,64 +737,16 @@ void nsMenuX::GetMenuPopupContent(nsICon
       NS_ADDREF(*aResult);
       return;
     }
   }
 
 }
 
 
-// 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");
-  
-  nsMenuObjectTypeX parentType = mParent->MenuObjectType();
-  if (parentType == eMenuBarObjectType) {
-    nsMenuBarX* menubarParent = static_cast<nsMenuBarX*>(mParent);
-    PRUint32 numMenus = menubarParent->GetMenuCount();
-
-    // 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++) {
-      nsMenuX* currMenu = menubarParent->GetMenuAt(i);
-      if (currMenu == this) {
-        // we found ourselves, break out
-        return NS_OK;
-      }
-
-      if (currMenu) {
-        nsIContent* menuContent = currMenu->Content();
-        if (menuContent->GetChildCount() > 0 &&
-            !nsMenuUtilsX::NodeIsHiddenOrCollapsed(menuContent)) {
-          ++(*outVisibleBefore);
-        }
-      }
-    }
-  }
-  else if (parentType == eSubmenuObjectType) {
-    // Find this menu among the children of my parent menu
-    nsMenuX* menuParent = static_cast<nsMenuX*>(mParent);
-    PRUint32 numItems = menuParent->GetItemCount();
-    for (PRUint32 i = 0; i < numItems; i++) {
-      // Using GetItemAt instead of GetVisibleItemAt to avoid O(N^2)
-      nsMenuObjectX* currItem = menuParent->GetItemAt(i);
-      if (currItem == this)
-        return NS_OK; // we found ourselves, break out
-      // If the node is visible increment the outparam.
-      if (MenuNodeIsVisible(currItem))
-        ++(*outVisibleBefore);
-    }
-  }
-  return NS_ERROR_FAILURE;
-}
-
-
 NSMenuItem* nsMenuX::NativeMenuItem()
 {
   return mNativeMenuItem;
 }
 
 
 //
 // nsChangeObserver
@@ -834,19 +774,19 @@ void nsMenuX::ObserveAttributeChanged(ns
     
     mContent->GetAttr(kNameSpaceID_None, nsWidgetAtoms::label, mLabel);
 
     // invalidate my parent. If we're a submenu parent, we have to rebuild
     // the parent menu in order for the changes to be picked up. If we're
     // a regular menu, just change the title and redraw the menubar.
     if (parentType == eMenuBarObjectType) {
       // reuse the existing menu, to avoid rebuilding the root menu bar.
-      NS_ASSERTION(mMacMenu, "nsMenuX::AttributeChanged: invalid menu handle.");
+      NS_ASSERTION(mNativeMenu, "nsMenuX::AttributeChanged: invalid menu handle.");
       NSString *newCocoaLabelString = nsMenuUtilsX::CreateTruncatedCocoaLabel(mLabel);
-      [mMacMenu setTitle:newCocoaLabelString];
+      [mNativeMenu setTitle:newCocoaLabelString];
       [newCocoaLabelString release];
     }
     else {
       static_cast<nsMenuX*>(mParent)->SetRebuild(PR_TRUE);
     }    
   }
   else if (aAttribute == nsWidgetAtoms::hidden || aAttribute == nsWidgetAtoms::collapsed) {
     SetRebuild(PR_TRUE);
@@ -864,21 +804,28 @@ void nsMenuX::ObserveAttributeChanged(ns
         // in the menu.
         if ([parentMenu indexOfItem:mNativeMenuItem] != -1)
           [parentMenu removeItem:mNativeMenuItem];
         mVisible = PR_FALSE;
       }
     }
     else {
       PRUint32 insertAfter = 0;
-      if (NS_SUCCEEDED(CountVisibleBefore(&insertAfter))) {
+      if (NS_SUCCEEDED(nsMenuUtilsX::CountVisibleBefore(mParent, this, &insertAfter))) {
         if (parentType == eMenuBarObjectType || parentType == eSubmenuObjectType) {
+          if (parentType == eMenuBarObjectType) {
+            // Before inserting we need to figure out if we should take the native
+            // application menu into account.
+            nsMenuBarX* mb = static_cast<nsMenuBarX*>(mParent);
+            if (mb->MenuContainsAppMenu())
+              insertAfter++;
+          }
           NSMenu* parentMenu = (NSMenu*)mParent->NativeData();
           [parentMenu insertItem:mNativeMenuItem atIndex:insertAfter];
-          [mNativeMenuItem setSubmenu:mMacMenu];
+          [mNativeMenuItem setSubmenu:mNativeMenu];
           mVisible = PR_TRUE;
         }
       }
     }
   }
   else if (aAttribute == nsWidgetAtoms::image) {
     SetupIcon();
   }
--- a/widget/src/xpwidgets/nsBaseWidget.h
+++ b/widget/src/xpwidgets/nsBaseWidget.h
@@ -132,16 +132,17 @@ public:
   NS_IMETHOD              SetIcon(const nsAString &anIconSpec);
   NS_IMETHOD              BeginSecureKeyboardInput();
   NS_IMETHOD              EndSecureKeyboardInput();
   NS_IMETHOD              SetWindowTitlebarColor(nscolor aColor, PRBool aActive);
   virtual void            ConvertToDeviceCoordinates(nscoord  &aX,nscoord &aY) {}
   virtual void            FreeNativeData(void * data, PRUint32 aDataType) {}
   NS_IMETHOD              BeginResizeDrag(nsGUIEvent* aEvent, PRInt32 aHorizontal, PRInt32 aVertical);
   virtual nsresult        ActivateNativeMenuItemAt(const nsAString& indexString) { return NS_ERROR_NOT_IMPLEMENTED; }
+  virtual nsresult        ForceNativeMenuReload() { return NS_ERROR_NOT_IMPLEMENTED; }
   NS_IMETHOD              ResetInputState() { return NS_ERROR_NOT_IMPLEMENTED; }
   NS_IMETHOD              SetIMEOpenState(PRBool aState) { return NS_ERROR_NOT_IMPLEMENTED; }
   NS_IMETHOD              GetIMEOpenState(PRBool* aState) { return NS_ERROR_NOT_IMPLEMENTED; }
   NS_IMETHOD              SetIMEEnabled(PRUint32 aState) { return NS_ERROR_NOT_IMPLEMENTED; }
   NS_IMETHOD              GetIMEEnabled(PRUint32* aState) { return NS_ERROR_NOT_IMPLEMENTED; }
   NS_IMETHOD              CancelIMEComposition() { return NS_ERROR_NOT_IMPLEMENTED; }
   NS_IMETHOD              GetToggledKeyState(PRUint32 aKeyCode, PRBool* aLEDState) { return NS_ERROR_NOT_IMPLEMENTED; }
 
--- a/widget/tests/native_menus_window.xul
+++ b/widget/tests/native_menus_window.xul
@@ -41,31 +41,41 @@
 
 <window id="NativeMenuWindow"
         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
         width="300"
         height="300"
         onload="onLoad();"
         title="Native Menu Test">
 
+  <command id="cmd_FooItem0" oncommand="executedCommandID = 'cmd_FooItem0';"/>
   <command id="cmd_FooItem1" oncommand="executedCommandID = 'cmd_FooItem1';"/>
-  <command id="cmd_FooItem2" oncommand="executedCommandID = 'cmd_FooItem2';"/>
+  <command id="cmd_BarItem0" oncommand="executedCommandID = 'cmd_BarItem0';"/>
   <command id="cmd_BarItem1" oncommand="executedCommandID = 'cmd_BarItem1';"/>
-  <command id="cmd_BarItem2" oncommand="executedCommandID = 'cmd_BarItem2';"/>
+  <command id="cmd_NewItem0" oncommand="executedCommandID = 'cmd_NewItem0';"/>
+  <command id="cmd_NewItem1" oncommand="executedCommandID = 'cmd_NewItem1';"/>
+  <command id="cmd_NewItem2" oncommand="executedCommandID = 'cmd_NewItem2';"/>
+  <command id="cmd_NewItem3" oncommand="executedCommandID = 'cmd_NewItem3';"/>
+  <command id="cmd_NewItem4" oncommand="executedCommandID = 'cmd_NewItem4';"/>
+  <command id="cmd_NewItem5" oncommand="executedCommandID = 'cmd_NewItem5';"/>
 
+  <!-- We do not modify any menus or menu items defined here in testing. These
+       serve as a baseline structure for us to test after other modifications.
+       We add children to the menubar defined here and test by modifying those
+       children. -->
   <menubar id="nativemenubar">
     <menu id="foo" label="Foo">
       <menupopup>
+        <menuitem label="FooItem0" command="cmd_FooItem0"/>
         <menuitem label="FooItem1" command="cmd_FooItem1"/>
-        <menuitem label="FooItem2" command="cmd_FooItem2"/>
         <menuseparator/>
         <menu label="Bar">
           <menupopup>
+            <menuitem label="BarItem0" command="cmd_BarItem0"/>
             <menuitem label="BarItem1" command="cmd_BarItem1"/>
-            <menuitem label="BarItem2" command="cmd_BarItem2"/>
           </menupopup>
         </menu>
       </menupopup>
     </menu>
   </menubar>
 
   <script type="application/javascript"><![CDATA[
 
@@ -73,43 +83,217 @@
       window.opener.wrappedJSObject.SimpleTest.ok(condition, message);
     }
 
     function onTestsFinished() {
       window.close();
       window.opener.wrappedJSObject.SimpleTest.finish();
     }
 
+    // We need to force a native menu reload before testing any dom changes
+    // because dom changes can affect the native menu system lazily.
+    function forceNativeMenuReload() {
+      netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
+      var utils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
+                                          getInterface(Components.interfaces.nsIDOMWindowUtils);
+      try {
+        utils.forceNativeMenuReload();
+      }
+      catch (e) {
+        dump(e + "\n");
+      }
+    }
+
     var executedCommandID = "";
 
     function testItem(location, targetID) {
       netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
       var utils = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
                                           getInterface(Components.interfaces.nsIDOMWindowUtils);
-
       var correctCommandHandler = false;
       try {
         utils.activateNativeMenuItemAt(location);
         correctCommandHandler = executedCommandID == targetID;
       }
       catch (e) {
         dump(e + "\n");
       }
       finally {
-        ok(correctCommandHandler, 'Command handler not run correctly');
         executedCommandID = "";
+        return correctCommandHandler;
       }
     }
 
+    function createXULMenuPopup() {
+      const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
+      var item = document.createElementNS(XUL_NS, "menupopup");
+      return item;
+    }
+
+    function createXULMenu(aLabel) {
+      const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
+      var item = document.createElementNS(XUL_NS, "menu");
+      item.setAttribute("label", aLabel);
+      return item;
+    }
+
+    function createXULMenuItem(aLabel, aCommandId) {
+      const XUL_NS = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
+      var item = document.createElementNS(XUL_NS, "menuitem");
+      item.setAttribute("label", aLabel);
+      item.setAttribute("command", aCommandId);
+      return item;
+    }
+
+    function runBaseMenuTests() {
+      return testItem("0|0", "cmd_FooItem0") &&
+             testItem("0|1", "cmd_FooItem1") &&
+             testItem("0|3|0", "cmd_BarItem0") &&
+             testItem("0|3|1", "cmd_BarItem1");
+    }
+
     function onLoad() {
       var _delayedOnLoad = function() {
-        testItem("0|0", "cmd_FooItem1");
-        testItem("0|1", "cmd_FooItem2");
-        testItem("0|3|0", "cmd_BarItem1");
-        testItem("0|3|1", "cmd_BarItem2");
-        onTestsFinished()
+        // First let's run the base menu tests.
+        ok(runBaseMenuTests());
+
+        // Set up some nodes that we'll use.
+        var menubarNode = document.getElementById("nativemenubar");
+        var newMenu0 = createXULMenu("NewMenu0");
+        var newMenu1 = createXULMenu("NewMenu1");
+        var newMenuPopup0 = createXULMenuPopup();
+        var newMenuPopup1 = createXULMenuPopup();
+        var newMenuItem0 = createXULMenuItem("NewMenuItem0", "cmd_NewItem0");
+        var newMenuItem1 = createXULMenuItem("NewMenuItem1", "cmd_NewItem1");
+        var newMenuItem2 = createXULMenuItem("NewMenuItem2", "cmd_NewItem2");
+        var newMenuItem3 = createXULMenuItem("NewMenuItem3", "cmd_NewItem3");
+        var newMenuItem4 = createXULMenuItem("NewMenuItem4", "cmd_NewItem4");
+        var newMenuItem5 = createXULMenuItem("NewMenuItem5", "cmd_NewItem5");
+
+        // Create another submenu with hierarchy via DOM manipulation.
+        // ******************
+        // * Foo * NewMenu0 * <- Menu bar 
+        // ******************
+        //       ****************
+        //       * NewMenuItem0 * <- NewMenu0 submenu
+        //       ****************
+        //       * NewMenuItem1 *
+        //       ****************
+        //       * NewMenuItem2 *
+        //       *******************************
+        //       * NewMenu1   > * NewMenuItem3 * <- NewMenu1 submenu
+        //       *******************************
+        //                      * NewMenuItem4 *
+        //                      ****************
+        //                      * NewMenuItem5 *
+        //                      ****************
+        menubarNode.appendChild(newMenu0);
+        newMenu0.appendChild(newMenuPopup0);
+        newMenuPopup0.appendChild(newMenuItem0);
+        newMenuPopup0.appendChild(newMenuItem1);
+        newMenuPopup0.appendChild(newMenuItem2);
+        newMenuPopup0.appendChild(newMenu1);
+        newMenu1.appendChild(newMenuPopup1);
+        newMenuPopup1.appendChild(newMenuItem3);
+        newMenuPopup1.appendChild(newMenuItem4);
+        newMenuPopup1.appendChild(newMenuItem5);
+
+        // Error strings.
+        var sa = "Command handler(s) should have activated";
+        var sna = "Command handler(s) should not have activated";
+
+        // Run basic tests again.
+        forceNativeMenuReload();
+        ok(runBaseMenuTests());
+
+        // Test middle items.
+        ok(testItem("1|1", "cmd_NewItem1"), sa);
+        ok(testItem("1|3|1", "cmd_NewItem4"), sa);
+
+        // Hide newMenu0.
+        newMenu0.setAttribute("hidden", "true");
+        forceNativeMenuReload();
+        ok(runBaseMenuTests(), sa); // the base menu should still be unhidden
+        ok(!testItem("1|0", ""), sna);
+        ok(!testItem("1|1", ""), sna);
+        ok(!testItem("1|2", ""), sna);
+        ok(!testItem("1|3|0", ""), sna);
+        ok(!testItem("1|3|1", ""), sna);
+        ok(!testItem("1|3|2", ""), sna);
+
+        // Show newMenu0.
+        newMenu0.setAttribute("hidden", "false");
+        forceNativeMenuReload();
+        ok(runBaseMenuTests(), sa);
+        ok(testItem("1|0", "cmd_NewItem0"), sa);
+        ok(testItem("1|1", "cmd_NewItem1"), sa);
+        ok(testItem("1|2", "cmd_NewItem2"), sa);
+        ok(testItem("1|3|0", "cmd_NewItem3"), sa);
+        ok(testItem("1|3|1", "cmd_NewItem4"), sa);
+        ok(testItem("1|3|2", "cmd_NewItem5"), sa);
+
+        // Hide items.
+        newMenuItem1.setAttribute("hidden", "true");
+        newMenuItem4.setAttribute("hidden", "true");
+        forceNativeMenuReload();
+        ok(runBaseMenuTests(), sa);
+        ok(testItem("1|0", "cmd_NewItem0"), sa);
+        ok(testItem("1|1", "cmd_NewItem2"), sa);
+        ok(!testItem("1|2", ""), sna);
+        ok(testItem("1|2|0", "cmd_NewItem3"), sa);
+        ok(testItem("1|2|1", "cmd_NewItem5"), sa);
+        ok(!testItem("1|2|2", ""), sna);
+
+        // Show items.
+        newMenuItem1.setAttribute("hidden", "false");
+        newMenuItem4.setAttribute("hidden", "false");
+        forceNativeMenuReload();
+        ok(runBaseMenuTests(), sa);
+        ok(testItem("1|0", "cmd_NewItem0"), sa);
+        ok(testItem("1|1", "cmd_NewItem1"), sa);
+        ok(testItem("1|2", "cmd_NewItem2"), sa);
+        ok(testItem("1|3|0", "cmd_NewItem3"), sa);
+        ok(testItem("1|3|1", "cmd_NewItem4"), sa);
+        ok(testItem("1|3|2", "cmd_NewItem5"), sa);
+
+        // At this point in the tests the state of the menus has been returned
+        // to the originally diagramed state.
+
+        // Remove menu.
+        menubarNode.removeChild(newMenu0);
+        forceNativeMenuReload();
+        ok(runBaseMenuTests(), sa);
+        ok(!testItem("1|0", ""), sna);
+        ok(!testItem("1|1", ""), sna);
+        ok(!testItem("1|2", ""), sna);
+        ok(!testItem("1|3|0", ""), sna);
+        ok(!testItem("1|3|1", ""), sna);
+        ok(!testItem("1|3|2", ""), sna);
+        // return state to original diagramed state
+        menubarNode.appendChild(newMenu0);
+
+        // Test for bug 447042, make sure that adding a menu node with no children
+        // to the menu bar and then adding another menu node with children works.
+        // Menus with no children don't get their native menu items shown and that
+        // caused internal arrays to get out of sync and an append crashed.
+        var tmpMenu0 = createXULMenu("tmpMenu0");
+        menubarNode.removeChild(newMenu0);
+        menubarNode.appendChild(tmpMenu0);
+        menubarNode.appendChild(newMenu0);
+        forceNativeMenuReload();
+        ok(runBaseMenuTests());
+        ok(testItem("1|0", "cmd_NewItem0"), sa);
+        ok(testItem("1|1", "cmd_NewItem1"), sa);
+        ok(testItem("1|2", "cmd_NewItem2"), sa);
+        ok(testItem("1|3|0", "cmd_NewItem3"), sa);
+        ok(testItem("1|3|1", "cmd_NewItem4"), sa);
+        ok(testItem("1|3|2", "cmd_NewItem5"), sa);
+        // return state to original diagramed state
+        menubarNode.removeChild(tmpMenu0);
+
+        onTestsFinished();
       }
 
       setTimeout(_delayedOnLoad, 1000);
-    }    
+    }
 
   ]]></script>
 </window>