481003 - Markup-based menu API is problematic

Status: RESOLVED FIXED

(Mozilla Labs :: Prism, defect)

Description

[Matthew Gertner]

1. Icon menus (dock, tray) don't load if the page can't load for some reason.
2. The menus go away as soon as you load a new page.

After much reflection I think the current markup-based approach where the menu lives in the webpage is wrong. We might want to have some way for pages to add items to the menu, but there should be some menu definition that lives independently of the webpage and stays consistent as the app is used under all circumstances.

The implication is that we will need to keep webapp.js or something like it. I think it makes sense for a webapp to have some code on the client that is separate from the app that runs in a normal browser.

Comment 1

[Matthew Gertner]

Attachment: Use a JS-based API instead of markup to define menus

One nice thing about this patch is that we no longer have two different menu implementations on Mac.

Comment 2

[Matthew Gertner]

Here's an example of how the new API is used (from load() in webapp.js):

  var icon = desktop.getApplicationIcon(window);
  icon.behavior = icon.HIDE_ON_MINIMIZE | icon.HIDE_ON_CLOSE;
  icon.title = properties.name;

  var menu = icon.menu;
  if (os == "WINNT") {
    menu.addMenuItem("webapp_open", "Open", function(e) { desktop.setZLevel(window, desktop.zLevelTop); });
  }
  menu.addMenuItem("webapp_prefs", "Preferences...", function(e) { host.showPreferences("paneWebApp"); });
  menu.addMenuItem("webapp_sanitize", "Clear Private Data...",
    function(e) { window.platform.clearPrivateData(); });
  if (os == "WINNT") {
    menu.addMenuItem("webapp_exit", "Exit", function(e) { window.platform.quit(); });
  }

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 364967 [details] [diff] [review]
Use a JS-based API instead of markup to define menus

Some significant code reorg. Have you tested this on mac and windows?

One of the big changes here, maybe the main one is AddMenuItem - which used to take the ID of an element and we built the menu from the attributes of the element. Now we are passing the ID, label and callback into the function and the ID is not used to look anything up. It's just assigned to the newly created <menuitem>. (it does remove a lot of code)

Comment 4

[Matthew Gertner]

Sending        runtime/chrome/content/preferences/connection.xul
Sending        runtime/chrome/content/preferences/preferences.xul
Sending        runtime/chrome/content/webrunner.js
Sending        runtime/components/public/nsINativeMenu.idl
Sending        runtime/components/src/Makefile.in
Deleting       runtime/components/src/mac/DOMElementWrapper.h
Deleting       runtime/components/src/mac/DOMElementWrapper.mm
Adding         runtime/components/src/mac/DOMEventListenerWrapper.h
Adding         runtime/components/src/mac/DOMEventListenerWrapper.mm
Sending        runtime/components/src/mac/Makefile.in
Sending        runtime/components/src/mac/nsCocoaMenu.h
Sending        runtime/components/src/mac/nsCocoaMenu.mm
Sending        runtime/components/src/mac/nsDesktopEnvironmentMac.mm
Sending        runtime/components/src/mac/nsDockTile.h
Sending        runtime/components/src/mac/nsDockTile.mm
Sending        runtime/components/src/nsDOMMenuBar.cpp
Sending        runtime/components/src/nsDOMMenuBar.h
Sending        runtime/components/src/windows/Makefile.in
Adding         runtime/components/src/windows/nsNativeMenu.cpp
Adding         runtime/components/src/windows/nsNativeMenu.h
Sending        runtime/components/src/windows/nsNotificationArea.cpp
Sending        runtime/components/src/windows/nsNotificationArea.h
Sending        runtime/components/src/windows/nsSystemMenu.cpp
Sending        runtime/components/src/windows/nsSystemMenu.h
Sending        runtime/modules/HostUI.jsm
Transmitting file data .......................
Committed revision 22849.

Comment 5

[Matthew Gertner]

I tested dock and tray menus (including submenus). I should test the menu bar as well although not much has changed there really. I don't see the ID thing as an issue. You're just providing a new ID for the menu item instead of referencing an existing item.

I think the idea of a page providing markup for menu items is interesting, but it requires a bit more thought and could be layered on top of the JS API we now have.