446352 - update native menu tests to test lazy update behavior

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Josh Aas]

We should update our native menu tests to test the lazy update behavior of native menus.

Right now after making changes via the DOM we tear down and rebuild the entire native menu bar from scratch. Instead we should tell particular menus to update themselves as if they were shown, via an API like 'ForceUpdateNativeMenu("0|0|0")'. That call should tell a particular menu to update itself, the testing script would be responsible for calling that on each menu in the parent hierarchy to a submenu if it wishes.

Comment 1

[Josh Aas]

Attachment: fix v0.5

This approach synthesizes carbon events, but it doesn't work because the carbon events don't get to their targets. Things seem to go wrong in the appkit carbon event handler, it just doesn't target the MenuRefs correctly. Carbon events still get sent correctly for genuine mouse events with this patch, so this could be a problem with using Carbon events in a Cocoa menu system. Backing this work up here.

Comment 2

[Josh Aas]

Attachment: fix v1.0

fun

Comment 3

[Markus Stange [:mstange]]

Comment on attachment 343224 [details] [diff] [review]
fix v1.0

I'm not too familiar with our menu system but your changes make sense to me.

>diff --git a/widget/src/cocoa/nsMenuBarX.mm b/widget/src/cocoa/nsMenuBarX.mm
...
>+  menuEvent.mCommand = (PRUint32)_NSGetCarbonMenu(static_cast<NSMenu*>(currentMenu->NativeData())); // this is a sanity check in the end, maybe find a way to run around it
...
>+            menuEvent.mCommand = (PRUint32)_NSGetCarbonMenu(static_cast<NSMenu*>(currentMenu->NativeData())); // this is a sanity check in the end, maybe find a way to run around it

These lines are a little long.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about getting rid of ForceNativeMenuReload and just using ForceUpdateNativeMenuAt with an empty string to do that?

Comment 5

[Josh Aas]

Attachment: fix v1.1

Comment 6

[Josh Aas]

Attachment: fix v1.2

Forgot to delete some obsolete comments.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  visible = 0;
+  for (unsigned int i = 1; currentMenu && i < indexCount; i++) {
+    targetIndex = [[indexes objectAtIndex:i] intValue];
+    for (unsigned int j = 0; j < currentMenu->GetItemCount(); j++) {
+      nsMenuObjectX* targetMenu = currentMenu->GetItemAt(j);
+      if (targetMenu && targetMenu->MenuObjectType() == eSubmenuObjectType) {
+        if (!nsMenuUtilsX::NodeIsHiddenOrCollapsed(targetMenu->Content())) {
+          visible++;
+          if (visible == (targetIndex + 1)) {

Shouldn't we be resetting visible *inside* the loop over i?

If that's a bug in this code, please add a test that fails because of that bug.

Comment 8

[Josh Aas]

Attachment: fix v1.3

That actually wasn't the only bug here, I made another silly mistake. This patch fixes the mistakes but doesn't add any more tests than I had added before. I'll have to do that later.

Comment 9

[Josh Aas]

landed on trunk

Comment 10

[Josh Aas]

Attachment: fix v1.4

This is what I actually landed, un-bitrotted.