Bug 1748815 - Don't dispatch separate runnables for menu item activation, just rely on the existing calls to MenuClosedAsync. r=bradwerth

ActivateItemAfterClosing is currently only called during automated tests.

Before this patch, the DoCommandRunnable could be run from two different places:
Either from the regular event loop (dispatched in `-[MOZMenuOpeningCoordinator _runMenu]`
once the menu event loop is exited), or from MenuClosedAsync, whichever happens first.

MenuClosedAsync always runs when the menu closes, so we can rely on
it being called after ActivateItemAfterClosing.

So we can simplify the code by just always firing the command event
from MenuClosedAsync.

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

widget/cocoa/nsMenuX.h

@@ -252,10 +252,15 @@ class nsMenuX final : public nsMenuParentX,
   // menuItemHit.
   RefPtr<mozilla::CancelableRunnable> mPendingAsyncMenuCloseRunnable;
 
-  // Any runnables for running asynchronous command events.
-  // These are only used during automated tests, via ActivateItemAfterClosing.
-  // We keep track of them here so that we can ensure they're run before popuphiding/popuphidden.
-  nsTArray<RefPtr<mozilla::Runnable>> mPendingCommandRunnables;
+  struct PendingCommandEvent {
+    RefPtr<nsMenuItemX> mMenuItem;
+    NSEventModifierFlags mModifiers;
+    int16_t mButton;
+  };
+
+  // Any pending command events.
+  // These are queued by ActivateItemAfterClosing and run by MenuClosedAsync.
+  nsTArray<PendingCommandEvent> mPendingCommandEvents;
 
   GeckoNSMenu* mNativeMenu = nil;     // [strong]
   MenuDelegate* mMenuDelegate = nil;  // [strong]

widget/cocoa/nsMenuX.mm

@@ -562,9 +562,9 @@ void nsMenuX::MenuClosedAsync() {
   }
 
   // If we have pending command events, run those first.
-  nsTArray<RefPtr<Runnable>> runnables = std::move(mPendingCommandRunnables);
-  for (auto& runnable : runnables) {
-    runnable->Run();
+  nsTArray<PendingCommandEvent> events = std::move(mPendingCommandEvents);
+  for (auto& event : events) {
+    event.mMenuItem->DoCommand(event.mModifiers, event.mButton);
   }
 
   // Make sure no item is highlighted.
@@ -594,41 +594,9 @@ void nsMenuX::MenuClosedAsync() {
 
 void nsMenuX::ActivateItemAfterClosing(RefPtr<nsMenuItemX>&& aItem, NSEventModifierFlags aModifiers,
                                        int16_t aButton) {
-  NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
-
-  class DoCommandRunnable final : public mozilla::Runnable {
-   public:
-    explicit DoCommandRunnable(RefPtr<nsMenuItemX>&& aItem, NSEventModifierFlags aModifiers,
-                               int16_t aButton)
-        : Runnable("DoCommandRunnable"),
-          mMenuItem(aItem),
-          mModifiers(aModifiers),
-          mButton(aButton) {}
-
-    nsresult Run() override {
-      if (mMenuItem) {
-        RefPtr<nsMenuItemX> menuItem = std::move(mMenuItem);
-        menuItem->DoCommand(mModifiers, mButton);
-      }
-      return NS_OK;
-    }
-
-   private:
-    RefPtr<nsMenuItemX> mMenuItem;  // cleared by Run()
-    NSEventModifierFlags mModifiers;
-    int16_t mButton;
-  };
-  RefPtr<Runnable> doCommandAsync = new DoCommandRunnable(std::move(aItem), aModifiers, aButton);
-  mPendingCommandRunnables.AppendElement(doCommandAsync);
-
-  // Delay the command event until after the menu's event loop has been exited, by using
-  // -[MOZMenuOpeningCoordinator runAfterMenuClosed:]. Otherwise, the runnable might potentially
-  // run inside the menu's nested event loop, and command event handlers can do arbitrary things
-  // like opening modal windows which spawn more nested event loops. This repeated nesting of event
-  // loops is something we'd like to avoid.
-  [MOZMenuOpeningCoordinator.sharedInstance runAfterMenuClosed:std::move(doCommandAsync)];
-
-  NS_OBJC_END_TRY_ABORT_BLOCK;
+  // Queue the event into mPendingCommandEvents. We will call aItem->DoCommand in MenuClosedAsync().
+  // We rely on the assumption that MenuClosedAsync will run soon.
+  mPendingCommandEvents.AppendElement(PendingCommandEvent{std::move(aItem), aModifiers, aButton});
 }
 
 bool nsMenuX::Close() {