Closed Bug 446352 Opened 12 years ago Closed 11 years ago

update native menu tests to test lazy update behavior

Categories

(Core :: Widget: Cocoa, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(1 file, 5 obsolete files)

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.
Attached patch fix v0.5 (obsolete) — Splinter Review
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.
Blocks: 456374
Attached patch fix v1.0 (obsolete) — Splinter Review
fun
Attachment #342905 - Attachment is obsolete: true
Attachment #343224 - Flags: review?(mstange)
Attachment #343224 - Flags: review?(mstange) → review+
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.
Attachment #343224 - Flags: superreview?(roc)
How about getting rid of ForceNativeMenuReload and just using ForceUpdateNativeMenuAt with an empty string to do that?
Attached patch fix v1.1 (obsolete) — Splinter Review
Attachment #343224 - Attachment is obsolete: true
Attachment #344093 - Flags: superreview?(roc)
Attachment #343224 - Flags: superreview?(roc)
Attachment #344093 - Flags: superreview?(roc) → superreview?(vladimir)
Attached patch fix v1.2 (obsolete) — Splinter Review
Forgot to delete some obsolete comments.
Attachment #344093 - Attachment is obsolete: true
Attachment #344093 - Flags: superreview?(vladimir)
Attachment #344176 - Flags: superreview?(roc)
+  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.
Attached patch fix v1.3 (obsolete) — Splinter Review
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.
Attachment #344176 - Attachment is obsolete: true
Attachment #344176 - Flags: superreview?(roc)
Attachment #344237 - Flags: superreview?(roc)
Attachment #344237 - Flags: superreview?(roc) → superreview+
landed on trunk
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Attached patch fix v1.4Splinter Review
This is what I actually landed, un-bitrotted.
Attachment #344237 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.