Bug 1043421 - Make native Mac menus observe mutations from the observed elements directly. r=smichaud
authorMarkus Stange <mstange@themasta.com>
Tue, 09 Sep 2014 17:14:46 +0200
changeset 204380 7c5219e1fa26337acf0076db4145f81df1cb5a15
parent 204379 6c0b0d329a5116587652e7cca4e1a8809c65b3d4
child 204381 cc2245e496e7649df27aaccccdd2a8801b38a5d7
push id27456
push userryanvm@gmail.com
push dateTue, 09 Sep 2014 23:26:49 +0000
treeherdermozilla-central@152ef25e89ae [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmichaud
bugs1043421
milestone35.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1043421 - Make native Mac menus observe mutations from the observed elements directly. r=smichaud
widget/cocoa/nsMenuBarX.mm
widget/cocoa/nsMenuGroupOwnerX.h
widget/cocoa/nsMenuGroupOwnerX.mm
widget/tests/native_menus_window.xul
widget/tests/standalone_native_menu_window.xul
--- a/widget/cocoa/nsMenuBarX.mm
+++ b/widget/cocoa/nsMenuBarX.mm
@@ -418,17 +418,17 @@ void nsMenuBarX::PaintMenuBarAfterDelay(
 // objects.  For example "key_selectAll".  Returns a value that can be
 // compared to the first character of [NSEvent charactersIgnoringModifiers]
 // when [NSEvent modifierFlags] == NSCommandKeyMask.
 char nsMenuBarX::GetLocalizedAccelKey(const char *shortcutID)
 {
   if (!sLastGeckoMenuBarPainted)
     return 0;
 
-  nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(sLastGeckoMenuBarPainted->mDocument));
+  nsCOMPtr<nsIDOMDocument> domDoc(do_QueryInterface(sLastGeckoMenuBarPainted->mContent->OwnerDoc()));
   if (!domDoc)
     return 0;
 
   NS_ConvertASCIItoUTF16 shortcutIDStr((const char *)shortcutID);
   nsCOMPtr<nsIDOMElement> shortcutElement;
   domDoc->GetElementById(shortcutIDStr, getter_AddRefs(shortcutElement));
   nsCOMPtr<nsIContent> shortcutContent = do_QueryInterface(shortcutElement);
   if (!shortcutContent)
--- a/widget/cocoa/nsMenuGroupOwnerX.h
+++ b/widget/cocoa/nsMenuGroupOwnerX.h
@@ -42,17 +42,16 @@ public:
 
 protected:
   virtual ~nsMenuGroupOwnerX();
 
   nsChangeObserver* LookupContentChangeObserver(nsIContent* aContent);
 
   uint32_t  mCurrentCommandID;  // unique command id (per menu-bar) to
                                 // give to next item that asks
-  nsIDocument* mDocument;       // pointer to document
 
   // stores observers for content change notification
   nsDataHashtable<nsPtrHashKey<nsIContent>, nsChangeObserver *> mContentToObserverTable;
 
   // stores mapping of command IDs to menu objects
   nsDataHashtable<nsUint32HashKey, nsMenuItemX *> mCommandToMenuObjectTable;
 };
 
--- a/widget/cocoa/nsMenuGroupOwnerX.mm
+++ b/widget/cocoa/nsMenuGroupOwnerX.mm
@@ -25,43 +25,34 @@
 #include "nsINode.h"
 
 using namespace mozilla;
 
 NS_IMPL_ISUPPORTS(nsMenuGroupOwnerX, nsIMutationObserver)
 
 
 nsMenuGroupOwnerX::nsMenuGroupOwnerX()
-: mCurrentCommandID(eCommand_ID_Last),
-  mDocument(nullptr)
+: mCurrentCommandID(eCommand_ID_Last)
 {
 }
 
 
 nsMenuGroupOwnerX::~nsMenuGroupOwnerX()
 {
-  // make sure we unregister ourselves as a document observer
-  if (mDocument)
-    mDocument->RemoveMutationObserver(this);
+  MOZ_ASSERT(mContentToObserverTable.Count() == 0, "have outstanding mutation observers!\n");
 }
 
 
 nsresult nsMenuGroupOwnerX::Create(nsIContent* aContent)
 {
   if (!aContent)
     return NS_ERROR_INVALID_ARG;
 
   mContent = aContent;
 
-  nsIDocument* doc = aContent->OwnerDoc();
-  if (!doc)
-    return NS_ERROR_FAILURE;
-  doc->AddMutationObserver(this);
-  mDocument = doc;
-
   return NS_OK;
 }
 
 
 //
 // nsIMutationObserver
 //
 
@@ -88,18 +79,16 @@ void nsMenuGroupOwnerX::ContentAppended(
   for (nsIContent* cur = aFirstNewContent; cur; cur = cur->GetNextSibling()) {
     ContentInserted(aDocument, aContainer, cur, 0);
   }
 }
 
 
 void nsMenuGroupOwnerX::NodeWillBeDestroyed(const nsINode * aNode)
 {
-  // our menu bar node is being destroyed
-  mDocument = nullptr;
 }
 
 
 void nsMenuGroupOwnerX::AttributeWillChange(nsIDocument* aDocument,
                                             dom::Element* aContent,
                                             int32_t aNameSpaceID,
                                             nsIAtom* aAttribute,
                                             int32_t aModType)
@@ -182,22 +171,28 @@ void nsMenuGroupOwnerX::ParentChainChang
 
 // For change management, we don't use a |nsSupportsHashtable| because
 // we know that the lifetime of all these items is bounded by the
 // lifetime of the menubar. No need to add any more strong refs to the
 // picture because the containment hierarchy already uses strong refs.
 void nsMenuGroupOwnerX::RegisterForContentChanges(nsIContent *aContent,
                                                   nsChangeObserver *aMenuObject)
 {
+  if (!mContentToObserverTable.Contains(aContent)) {
+    aContent->AddMutationObserver(this);
+  }
   mContentToObserverTable.Put(aContent, aMenuObject);
 }
 
 
 void nsMenuGroupOwnerX::UnregisterForContentChanges(nsIContent *aContent)
 {
+  if (mContentToObserverTable.Contains(aContent)) {
+    aContent->RemoveMutationObserver(this);
+  }
   mContentToObserverTable.Remove(aContent);
 }
 
 
 nsChangeObserver* nsMenuGroupOwnerX::LookupContentChangeObserver(nsIContent* aContent)
 {
   nsChangeObserver * result;
   if (mContentToObserverTable.Get(aContent, &result))
--- a/widget/tests/native_menus_window.xul
+++ b/widget/tests/native_menus_window.xul
@@ -220,16 +220,24 @@
         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.
 
+        // Test command disabling
+        var cmd_NewItem0 = document.getElementById("cmd_NewItem0");
+        ok(testItem("1|0", "cmd_NewItem0"), sa);
+        cmd_NewItem0.setAttribute("disabled", "true");
+        ok(!testItem("1|0", "cmd_NewItem0"), sna);
+        cmd_NewItem0.removeAttribute("disabled");
+        ok(testItem("1|0", "cmd_NewItem0"), sa);
+
         // Remove menu.
         menubarNode.removeChild(newMenu0);
         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);
--- a/widget/tests/standalone_native_menu_window.xul
+++ b/widget/tests/standalone_native_menu_window.xul
@@ -97,37 +97,86 @@
     function runBaseMenuTests(menu) {
       menu.forceUpdateNativeMenuAt("0|3");
       return testItem(menu, "0|0", "cmd_FooItem0") &&
              testItem(menu, "0|1", "cmd_FooItem1") &&
              testItem(menu, "0|3|0", "cmd_BarItem0") &&
              testItem(menu, "0|3|1", "cmd_BarItem1");
     }
 
+    function createStandaloneNativeMenu(menuNode) {
+      try {
+        let Cc = Components.classes;
+        let Ci = Components.interfaces;
+        let menu = Cc["@mozilla.org/widget/standalonenativemenu;1"].createInstance(Ci.nsIStandaloneNativeMenu);
+        menu.init(menuNode);
+        return menu;
+      } catch (e) {
+        ok(false, "Failed creating nsIStandaloneNativeMenu instance");
+        throw e;
+      }
+    }
+
+    function runDetachedMenuTests(addMenupopupBeforeCreatingSNM) {
+      let menu = createXULMenu("Detached menu");
+      menu.setAttribute("image", 'data:image/svg+xml,<svg%20xmlns="http://www.w3.org/2000/svg"%20width="32"%20height="32"><circle%20cx="16"%20cy="16"%20r="16"/></svg>');
+      let menupopup = createXULMenuPopup();
+
+      let popupShowingFired = false;
+      let itemActivated = false;
+
+      menupopup.addEventListener("popupshowing", function (e) {
+        popupShowingFired = true;
+
+        let menuitem = document.createElementNS(XUL_NS, "menuitem");
+        menuitem.setAttribute("label", "detached menu item");
+        menuitem.addEventListener("command", function (e) {
+          itemActivated = true;
+        })
+        menupopup.appendChild(menuitem);
+      })
+
+      // It shouldn't make a difference whether the menupopup is added to the
+      // menu element before or after we create the nsIStandaloneNativeMenu
+      // instance with it. We test both orders by calling this function twice
+      // with different values for addMenupopupBeforeCreatingSNM.
+
+      var menuSNM = null; // the nsIStandaloneNativeMenu object for "menu"
+      if (addMenupopupBeforeCreatingSNM) {
+        menu.appendChild(menupopup);
+        menuSNM = createStandaloneNativeMenu(menu);
+      } else {
+        menuSNM = createStandaloneNativeMenu(menu);
+        menu.appendChild(menupopup);
+      }
+
+      try {
+        ok(!popupShowingFired, "popupshowing shouldn't have fired before our call to menuWillOpen()");
+        menuSNM.menuWillOpen();
+        ok(popupShowingFired, "calling menuWillOpen() should have notified our popupshowing listener");
+
+        ok(!itemActivated, "our dynamically-added menuitem shouldn't have been activated yet");
+        menuSNM.activateNativeMenuItemAt("0");
+        ok(itemActivated, "the new menu item should have been activated now");
+      } catch (ex) {
+        ok(false, "dynamic menu test failed: " + ex);
+      }
+    }
+
     function onLoad() {
       var _delayedOnLoad = function() {
-        var Cc = Components.classes;
-        var Ci = Components.interfaces;
+        try {
 
         var menuNode = document.getElementById("standalonenativemenu");
-        var menu;
-        try {
-          menu = Cc["@mozilla.org/widget/standalonenativemenu;1"].createInstance(Ci.nsIStandaloneNativeMenu);
-          menu.init(menuNode);
-        } catch (e) {
-          ok(false, "Create nsIStandaloneNativeMenu instance");
-          onTestsFinished();
-          return;
-        }
+        var menu = createStandaloneNativeMenu(menuNode);
 
         // First let's run the base menu tests.
         ok(runBaseMenuTests(menu), "base tests #1");
 
         // 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");
@@ -262,16 +311,24 @@
         newMenuItem2.setAttribute("hidden", "true");
         newMenu1.setAttribute("hidden", "true");
         menu.forceUpdateNativeMenuAt("1");
         newMenuItem1.setAttribute("hidden", "false");
         newMenuItem2.setAttribute("hidden", "false");
         newMenu1.setAttribute("hidden", "false");
         menu.forceUpdateNativeMenuAt("1");
 
-        onTestsFinished();
+        // Run tests where the menu nodes are not in the document's node tree.
+        runDetachedMenuTests(false);
+        runDetachedMenuTests(true);
+
+        } catch (e) {
+          ok(false, "Caught an exception: " + e);
+        } finally {
+          onTestsFinished();
+        }
       }
 
       setTimeout(_delayedOnLoad, 1000);
     }
 
   ]]></script>
 </window>