Bug 1705842 - Combine the two methods and properly update mVisibleItemsCount, and add some asserts. r=harry

Differential Revision: https://phabricator.services.mozilla.com/D112450

widget/cocoa/nsMenuBarX.h

@@ -115,12 +115,8 @@ class nsMenuBarX : public nsMenuParentX, public nsChangeObserver, public mozilla
   bool PerformKeyEquivalent(NSEvent* aEvent);
   GeckoNSMenu* NativeNSMenu() { return mNativeMenu; }
 
-  // If aChild is one of our child menus, insert aChild's native menu item in our native menu at the
-  // right location.
-  void InsertChildNativeMenuItem(const MenuChild& aChild) override;
-
-  // Remove aChild's native menu item from our native menu.
-  void RemoveChildNativeMenuItem(const MenuChild& aChild) override;
+  // nsMenuParentX
+  void MenuChildChangedVisibility(const MenuChild& aChild, bool aIsVisible) override;
 
  protected:
   virtual ~nsMenuBarX();

widget/cocoa/nsMenuBarX.mm

@@ -224,7 +224,7 @@ void nsMenuBarX::InsertMenuAtIndex(RefPtr<nsMenuX>&& aMenu, uint32_t aIndex) {
   // hook up submenus
   RefPtr<nsIContent> menuContent = aMenu->Content();
   if (menuContent->GetChildCount() > 0 && !nsMenuUtilsX::NodeIsHiddenOrCollapsed(menuContent)) {
-    InsertChildNativeMenuItem(MenuChild(aMenu));
+    MenuChildChangedVisibility(MenuChild(aMenu), true);
   }
 
   NS_OBJC_END_TRY_ABORT_BLOCK;
@@ -448,18 +448,14 @@ bool nsMenuBarX::PerformKeyEquivalent(NSEvent* aEvent) {
   return [mNativeMenu performSuperKeyEquivalent:aEvent];
 }
 
-void nsMenuBarX::InsertChildNativeMenuItem(const MenuChild& aChild) {
-  MOZ_RELEASE_ASSERT(aChild.is<RefPtr<nsMenuX>>(), "nsMenuBarX only has nsMenuX children");
-  const RefPtr<nsMenuX>& child = aChild.as<RefPtr<nsMenuX>>();
-  NSInteger insertionPoint = CalculateNativeInsertionPoint(child);
-  [mNativeMenu insertItem:child->NativeNSMenuItem() atIndex:insertionPoint];
-}
-
-void nsMenuBarX::RemoveChildNativeMenuItem(const MenuChild& aChild) {
+void nsMenuBarX::MenuChildChangedVisibility(const MenuChild& aChild, bool aIsVisible) {
   MOZ_RELEASE_ASSERT(aChild.is<RefPtr<nsMenuX>>(), "nsMenuBarX only has nsMenuX children");
   const RefPtr<nsMenuX>& child = aChild.as<RefPtr<nsMenuX>>();
   NSMenuItem* item = child->NativeNSMenuItem();
-  if ([mNativeMenu indexOfItem:item] != -1) {
+  if (aIsVisible) {
+    NSInteger insertionPoint = CalculateNativeInsertionPoint(child);
+    [mNativeMenu insertItem:child->NativeNSMenuItem() atIndex:insertionPoint];
+  } else if ([mNativeMenu indexOfItem:item] != -1) {
     [mNativeMenu removeItem:item];
   }
 }

widget/cocoa/nsMenuParentX.h

@@ -21,12 +21,11 @@ class nsMenuParentX {
   // XXXmstange double-check that this is still needed
   virtual nsMenuBarX* AsMenuBar() { return nullptr; }
 
-  // If aChild is one of our child menus, insert aChild's native menu item in
-  // our native menu at the right location.
-  virtual void InsertChildNativeMenuItem(const MenuChild& aChild) = 0;
-
-  // Remove aChild's native menu item froum our native menu.
-  virtual void RemoveChildNativeMenuItem(const MenuChild& aChild) = 0;
+  // Called when aChild becomes visible or hidden, so that the parent can insert
+  // or remove the child's native menu item from its NSMenu and update its state
+  // of visible items.
+  virtual void MenuChildChangedVisibility(const MenuChild& aChild,
+                                          bool aIsVisible) = 0;
 };
 
 #endif  // nsMenuParentX_h_

widget/cocoa/nsMenuX.h

@@ -140,12 +140,8 @@ class nsMenuX final : public nsMenuParentX,
   void SetIconListener(nsMenuItemIconX::Listener* aListener) { mIconListener = aListener; }
   void ClearIconListener() { mIconListener = nullptr; }
 
-  // If aChild is one of our child menus, insert aChild's native menu item in our native menu at the
-  // right location.
-  void InsertChildNativeMenuItem(const MenuChild& aChild) override;
-
-  // Remove aChild's native menu item froum our native menu.
-  void RemoveChildNativeMenuItem(const MenuChild& aChild) override;
+  // nsMenuParentX
+  void MenuChildChangedVisibility(const MenuChild& aChild, bool aIsVisible) override;
 
   void Dump(uint32_t aIndent) const;
 

widget/cocoa/nsMenuX.mm

@@ -843,11 +843,7 @@ void nsMenuX::ObserveAttributeChanged(dom::Document* aDocument, nsIContent* aCon
 
     if (mParent) {
       RefPtr<nsMenuX> self = this;
-      if (newVisible) {
-        mParent->InsertChildNativeMenuItem(MenuChild(self));
-      } else {
-        mParent->RemoveChildNativeMenuItem(MenuChild(self));
-      }
+      mParent->MenuChildChangedVisibility(MenuChild(self), newVisible);
     }
     mVisible = newVisible;
   } else if (aAttribute == nsGkAtoms::image) {
@@ -888,21 +884,26 @@ void nsMenuX::IconUpdated() {
   }
 }
 
-void nsMenuX::InsertChildNativeMenuItem(const MenuChild& aChild) {
-  NSInteger insertionPoint = CalculateNativeInsertionPoint(aChild);
+void nsMenuX::MenuChildChangedVisibility(const MenuChild& aChild, bool aIsVisible) {
+  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
+
   NSMenuItem* nativeItem = aChild.match(
       [](const RefPtr<nsMenuX>& aMenu) { return aMenu->NativeNSMenuItem(); },
       [](const RefPtr<nsMenuItemX>& aMenuItem) { return aMenuItem->NativeNSMenuItem(); });
+  if (aIsVisible) {
+    MOZ_RELEASE_ASSERT(!nativeItem.menu,
+                       "The native item should not be in a menu while it is hidden");
+    NSInteger insertionPoint = CalculateNativeInsertionPoint(aChild);
     [mNativeMenu insertItem:nativeItem atIndex:insertionPoint];
+    mVisibleItemsCount++;
+  } else {
+    MOZ_RELEASE_ASSERT([mNativeMenu indexOfItem:nativeItem] != -1,
+                       "The native item should be in this menu while it is visible");
+    [mNativeMenu removeItem:nativeItem];
+    mVisibleItemsCount--;
   }
 
-void nsMenuX::RemoveChildNativeMenuItem(const MenuChild& aChild) {
-  NSMenuItem* nativeItem = aChild.match(
-      [](const RefPtr<nsMenuX>& aMenu) { return aMenu->NativeNSMenuItem(); },
-      [](const RefPtr<nsMenuItemX>& aMenuItem) { return aMenuItem->NativeNSMenuItem(); });
-  if ([mNativeMenu indexOfItem:nativeItem] != -1) {
-    [mNativeMenu removeItem:nativeItem];
-  }
+  NS_OBJC_END_TRY_ABORT_BLOCK;
 }
 
 NSInteger nsMenuX::CalculateNativeInsertionPoint(const MenuChild& aChild) {
@@ -915,9 +916,7 @@ NSInteger nsMenuX::CalculateNativeInsertionPoint(const MenuChild& aChild) {
     NSMenuItem* nativeItem = currItem.match(
         [](const RefPtr<nsMenuX>& aMenu) { return aMenu->NativeNSMenuItem(); },
         [](const RefPtr<nsMenuItemX>& aMenuItem) { return aMenuItem->NativeNSMenuItem(); });
-    // Only count items that are inside a menu.
-    // XXXmstange Not sure what would cause free-standing items. Maybe for collapsed/hidden menus?
-    // In that case, an nsMenuX::IsVisible() method would be better.
+    // Only count visible items.
     if (nativeItem.menu) {
       insertionPoint++;
     }