Dock menu support for Mac OS X (add "New Window" menu item)

VERIFIED FIXED in Firefox 3.7a2

Status

()

enhancement
VERIFIED FIXED
12 years ago
7 years ago

People

(Reporter: teoli, Assigned: tom.dyas)

Tracking

(Blocks 2 bugs)

Trunk
Firefox 3.7a2
All
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(status2.0 wanted)

Details

(Whiteboard: parity-safari)

Attachments

(7 attachments, 19 obsolete attachments)

3.09 KB, patch
jaas
: review+
Details | Diff | Splinter Review
35.49 KB, patch
jaas
: review+
Details | Diff | Splinter Review
7.06 KB, patch
jaas
: review+
Details | Diff | Splinter Review
40.73 KB, patch
jaas
: review+
Details | Diff | Splinter Review
2.71 KB, patch
jaas
: review+
Details | Diff | Splinter Review
139.54 KB, image/png
Details
1.33 KB, patch
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fr-FR; rv:1.9b3pre) Gecko/2008020304 Minefield/3.0b3pre
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; fr-FR; rv:1.9b3pre) Gecko/2008020304 Minefield/3.0b3pre

In Leopard, there is now Spaces to get virtual desktop.

When being in a Space without a Firefox window, there is no easy way to open a new Firefox window there. (The easiest way is to switch to Firefox, to open a new window, to go to the Spaces mosaic, to move the new window is the original space and then to move to that space).

Safari just added a "Open new window..." in its Dock menu.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Whiteboard: parity-safari
Version: unspecified → Trunk
Duplicate of this bug: 418461
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 435680
Flags: wanted-firefox3.1+
Target Milestone: --- → Firefox 3.1
This patch modifies the applicationDockMenu method of toolkit/xre/MacApplicationDelegate.mm to add custom menu items defined in XUL to the Dock Menu.  I patched browser.xul with some example items.

Unfortunately, this patch crashes the browser whenever the dock menu is activated for a second time.  (The javascript code is triggered properly the first time around so at least that is progress!)  All of my debugging printf's are triggered so the crash is happening somewhere else.  (And I still need to figure out the reference-counting situation ...)

Anyone have any thoughts?
The Prism project has code that solves this problem as well.  I'll need to look
at their code to see if it has any applicability to this situation.

See: http://viewvc.svn.mozilla.org/vc/projects/webrunner/trunk/runtime/component/src/mac/nsDesktopEnvironmentMac.mm?view=markup
Blocks: FF2SM
The patch now works (always helps to do proper reference counting between Cocoa and XPCOM!).  The toolkit/xre code exposes the nsIMacDockSupport interface to allow XUL applications to add XUL menu nodes to be included in the dock menu.  I modified the Firefox code to just add a special "menu_mac_dockmenu" menupopup element.

I would like comments on the API especially with the Prism folks. I made it Mac-specific because there is a fundamental difference between the Mac dock menu and the Windows system tray context menu IMHO.

Some nits:

* There is no code sharing with the Mac-specific menu code in widget/src/cocoa.  MacApplicationDelegate has to rebuild the menu items each time and the dock menu only supports a subset of what normal menu items support.

* That said, this patch is not feature complete.  For example, it doesn't check command nodes to determine whether a menu item should be disabled.
Attachment #350866 - Attachment is obsolete: true
Side note: Prism unifies adding items to dock menu and Windows system tray.  Hence, my comment above about making this API Mac-specific.
(In reply to comment #5)
> I would like comments on the API especially with the Prism folks. I made it
> Mac-specific because there is a fundamental difference between the Mac dock
> menu and the Windows system tray context menu IMHO.

Well there is a fundamental difference between the two implementations, which is that we use HTML to specify the menu whereas you use XUL. Nonetheless, I see ample possibilities for harmonization that I would personally like to explore. After all, at the end of the day we are just firing off DOM events to an event target which could be an HTML or a XUL element.

I don't see any reason to make this Mac-specific at the interface level. The basic functionality needed to add/remove items from menus is the same across platforms. What would you think about using our nsINativeMenu interface (http://mxr.mozilla.org/mozillasvn/source/projects/webrunner/trunk/runtime/components/public/nsINativeMenu.idl) or some derivative?

Also, the bulk (or all?) of this seems more like platform functionality than browser functionality. I think you hit the nail on the head when you mention the menu code in widget. The underlying code for building the menu belongs there, I think, and if there is some way to integrate it with the existing code that would be ideal (though I understand that that would entail additional challenges). Whether or not it is combined with existing menu code, it would be hugely beneficial to have it available to other XULRunner apps including Prism. If we don't want to touch widget we could use toolkit instead.
(In reply to comment #7)
> Well there is a fundamental difference between the two implementations, which
> is that we use HTML to specify the menu whereas you use XUL. Nonetheless, I see
> ample possibilities for harmonization that I would personally like to explore.
> After all, at the end of the day we are just firing off DOM events to an event
> target which could be an HTML or a XUL element.

I picked XUL since the menus of all Gecko applications are defined using XUL.  From my understanding of Prism, the feature to convert HTML command elements into menu items seems to be specific to Prism and not something that most XUL-based applications would ordinarily do. Is this correct?

> I don't see any reason to make this Mac-specific at the interface level. The
> basic functionality needed to add/remove items from menus is the same across
> platforms. What would you think about using our nsINativeMenu interface
> (http://mxr.mozilla.org/mozillasvn/source/projects/webrunner/trunk/runtime/components/public/nsINativeMenu.idl)
> or some derivative?

Agreed.  I would modify the interface, however, so that it initializes itself from a XUL node passed in as a nsIContent and not as an ID of an element in a implicit document.  Also, this would let me leverage the widget/src/cocoa menu code very easily at it expects XUL menu nodes.

> Also, the bulk (or all?) of this seems more like platform functionality than
> browser functionality. I think you hit the nail on the head when you mention
> the menu code in widget. The underlying code for building the menu belongs
> there, I think, and if there is some way to integrate it with the existing code
> that would be ideal (though I understand that that would entail additional
> challenges). Whether or not it is combined with existing menu code, it would be
> hugely beneficial to have it available to other XULRunner apps including Prism.
> If we don't want to touch widget we could use toolkit instead.

My patch already puts the bulk of the work into toolkit.  The browser code just uses the API that the toolkit code exposes (nsIMacDockSupport).

I'm going to give some thought about how the interface should look from the widget library to the rest of Gecko.
(In reply to comment #8)
> I picked XUL since the menus of all Gecko applications are defined using XUL. 
> From my understanding of Prism, the feature to convert HTML command elements
> into menu items seems to be specific to Prism and not something that most
> XUL-based applications would ordinarily do. Is this correct?

Sure, different requirements call for different design decisions.

> > I don't see any reason to make this Mac-specific at the interface level. The
> > basic functionality needed to add/remove items from menus is the same across
> > platforms. What would you think about using our nsINativeMenu interface
> > (http://mxr.mozilla.org/mozillasvn/source/projects/webrunner/trunk/runtime/components/public/nsINativeMenu.idl)
> > or some derivative?
> 
> Agreed.  I would modify the interface, however, so that it initializes itself
> from a XUL node passed in as a nsIContent and not as an ID of an element in a
> implicit document.  Also, this would let me leverage the widget/src/cocoa menu
> code very easily at it expects XUL menu nodes.

I agree that using an ID is a bit weird. I frankly can't remember why I did it that was (it was a while ago). But I much prefer using nsIDOMElement to nsIContent. I don't think nsIContent should be exposed at the API level.

> My patch already puts the bulk of the work into toolkit.  The browser code just
> uses the API that the toolkit code exposes (nsIMacDockSupport).
> 
> I'm going to give some thought about how the interface should look from the
> widget library to the rest of Gecko.

Great. Please ping me here when you have a new version and I'll evaluate the feasibility of moving the Prism code to use your implementation once it is deployed in XULRunner.
Target Milestone: Firefox 3.1 → Firefox 3.5
Duplicate of this bug: 497162
The patch has been rewritten to use new component nsINativeMenu.  Menu items show up in the dock menu, but there is a bug in activating some of the items.  (Lots of debugging printf()'s still in place as well.)

Comments wanted especially on the new interfaces.

Major changes:
* Refactored parts of nsMenuBarX into new class nsMenuOwnerX that is now the common ancestor of nsMenuBarX and nsNativeMenu
* Created nsINativeMenu
* Created nsIMacDockSupport
Attachment #355803 - Attachment is obsolete: true
Dock menu items now work correctly.  There is still a bug where radio items do not uncheck their siblings when selected.
Attachment #384241 - Attachment is obsolete: true
Duplicate of this bug: 502401
Rebased patch to apply to today's trunk.
Attachment #384634 - Attachment is obsolete: true
(In reply to comment #14)
> Created an attachment (id=386888) [details]
> Dock menu support v5 (work in progress)
> 
> Rebased patch to apply to today's trunk.

Definitely looks promising. I'm traveling right now so I can't do this justice, but a few random comments:

- You should probably check with the browser guys to be sure, but it might be better to put Mac-specific stuff (like IDL files) into their own subdirectory.

- Can you explain in a bit more detail the rationale for giving nsMenuBarX and nsNativeMenu a common parent class?

- Is some stuff missing from the patch? For example, I couldn't find nsNativeMenu.idl.
(In reply to comment #15)
> Definitely looks promising. I'm traveling right now so I can't do this justice,
> but a few random comments:
> 
> - You should probably check with the browser guys to be sure, but it might be
> better to put Mac-specific stuff (like IDL files) into their own subdirectory.

The only Mac-specific IDL is nsIMacDockSupport.idl so I'll keep in mind to check with the widget library maintainers about it.  nsINativeMenu.idl should be usable by Windows as well.

The browser initialization code is in the Mac-specific function already.

> - Can you explain in a bit more detail the rationale for giving nsMenuBarX and
> nsNativeMenu a common parent class?

nsMenuBarX did two things originally: (1) it managed its submenus and (2) it monitored DOM mutation events on behalf of its subordinate objects.  A reference to the top-level nsMenuBarX for a particular menu bar was passed to all subordinate nsMenuItemX and nsMenuX objects so they could register for DOM mutation observation.

Introducing nsNativeMenuX breaks this assumption since it is not a nsMenuBarX.  I refactored the mutation observation code out of nsMenuBarX and hoisted it into nsMenuOwnerX.  nsNativeMenuX now just derives from that nsMenuOwnerX so the subordinate objects don't need to know whether they are part of a menu bar or independent native menu. 

> - Is some stuff missing from the patch? For example, I couldn't find
> nsNativeMenu.idl.

Forgot to hg add nsINativeMenu.idl.  I'll check if there are other missing files.  It basically just contains an Init() method and a non-scriptable attribute to retrieve the underlying native NSMenu.
Duplicate of this bug: 506923
Assignee: nobody → tdyas
Status: NEW → ASSIGNED
Hardware: PowerPC → All
Target Milestone: Firefox 3.5 → ---
Updated the patch to apply to the current trunk. This basically works, except for unchecking radio siblings. For some reason, a nsMenuBarX is receiving the change notifications instead of nsNativeMenu whenever a radio node is checked. Still left in tons of debugging stuff which will have to be removed for any final patch.
Attachment #386888 - Attachment is obsolete: true
Posted patch Dock menu support v7 (obsolete) — Splinter Review
Fixed some bugs and removed debugging code.  Should be ready for review.

Matthew: I'd like your thoughts on the nsINativeMenu interface.
Attachment #410669 - Attachment is obsolete: true
Attachment #415008 - Flags: review?(joshmoz)
(In reply to comment #19)
> Matthew: I'd like your thoughts on the nsINativeMenu interface.

Well FWIW we moved away from generating the menu directly from DOM. This was too restrictive since we want to be able to generate menus even in cases where is no DOM (e.g. when a web page can't load, but you still want to have a dock menu). So I changed the interface to manipulate the menu items programmatically rather than through markup: http://mxr.mozilla.org/mozillasvn/source/projects/webrunner/components/public/nsINativeMenu.idl

I am very much in favor of harmonizing these things, in which case I think you might consider using a similar approach and putting the code to map from XUL into this interface somewhere else. Otherwise you have something which is dependent on the menu being declared in markup.
I believe you are conflating the chrome and content documents. (Prism generates the Prism dock menu entries from the Web application's content document?)

Under my patch, you would store an initially empty dock menu in your webrunner.xul as a |menupopup| in a |popupset| (or with entries if there are basic menu entries you wanted).  When the applicable content document loads, you would create new dock menu entries by adding |menuitem| nodes to the dock menu node in the chrome document.  As webrunner.xul is a chrome document, it should always be available even when no content has been loaded.

Also, using XUL is actually more expressive because your version of nsINativeMenu.idl only exposes adding and removing simple menu entries.  My implementation allows you to hide/unhide items, enable/disable items, and implement checkboxes and radio groups; basically all of the XUL menu features (to the extent supported by the Mac dock of course).

Triggering a dock menu entry will invoke whatever |command| or |oncommand| attributes have been assigned to the |menuitem| nodes.  These should be in chrome and would then call into the content document as needed.
The implementation takes advantage of the fact that the Mac menu code uses an nsIMutationObserver to update the menus dynamically in response to changes in the XUL DOM.  So you can use the chrome document's DOM to update the dock menu once it has been installed.
I understand your point. My suggestion was simply to have two levels of interface, one that looks like yours (high-level based on DOM) and one that looks like mine (low-level based on API calls). This would enable us to use the same low-level interface for both chrome and content-based menus, with your high-level interface calling the low-level interface.

Of course you are right that the low-level interface should be fleshed out with additional methods for hide/unhide, enable/disable, etc.
(In reply to comment #23)
> I understand your point. My suggestion was simply to have two levels of
> interface, one that looks like yours (high-level based on DOM) and one that
> looks like mine (low-level based on API calls). This would enable us to use the
> same low-level interface for both chrome and content-based menus, with your
> high-level interface calling the low-level interface.

What code would be using this low-level interface? (HTML content or Prism chrome?)
Posted patch Dock menu support v8 (obsolete) — Splinter Review
A few changes:

- Update for change to nsIMutationObserver interface.
- Trust in getService to only instantiate nsIMacDockSupport service once.
- nsINativeMenu.idl: Improved comments and better names for methods.
- nsNativeMenu.mm: Check the tag of the passed-in DOM element.
Attachment #415008 - Attachment is obsolete: true
Attachment #417814 - Flags: review?(joshmoz)
Attachment #415008 - Flags: review?(joshmoz)
Duplicate of this bug: 536222
(In reply to comment #24)
> What code would be using this low-level interface? (HTML content or Prism
> chrome?)

Currently I use this low-level interface for modifying menus in Prism chrome. I can't use a markup-based interface since a web app might want to modify the menus even if there isn't any markup (e.g. a text file is loaded into the browser window).
(In reply to comment #27)
> (In reply to comment #24)
> > What code would be using this low-level interface? (HTML content or Prism
> > chrome?)
> 
> Currently I use this low-level interface for modifying menus in Prism chrome. I
> can't use a markup-based interface since a web app might want to modify the
> menus even if there isn't any markup (e.g. a text file is loaded into the
> browser window).


I think there is still a disconnect between us.

If a text file is loaded into the browser window, isn't chrome/content/webrunner.xul still loaded and active in order to define the window in which that text file is displayed?  You would dynamically modify the nodes under a menupopup node in the webrunner.xul to define the dock menu; the content document would not (and from a security standpoint probably should not) be involved.  The chrome DOM should be available to your chrome code even if a text document were loaded.  If you look at the Firefox portion of my patch, you will see that the "menu_mac_dockmenu" node is in an overlay over browser.xul.

You can expose a low-level interface to content from your webrunner.xul chrome.  This chrome code would simply make a new menuitem node using the chrome DOM (not the content DOM) and add it to the menupopup setup previously.  The code in my patch will notified of the change and update the dock menu accordingly.
Comment on attachment 417814 [details] [diff] [review]
Dock menu support v8

I think we should do this in 3 steps:

1. Hash table fixes
2. Hoist DOM observer stuff into new class separate from nsMenuBarX
3. Implement dock menu

Please use NULL, nil, or nsnull instead of 0 where you can, when assigning to a pointer.
Attachment #417814 - Flags: review?(joshmoz)
Also, I like the DOM-based API as it is in your patch.
(In reply to comment #28)
> I think there is still a disconnect between us.
> 
> If a text file is loaded into the browser window, isn't
> chrome/content/webrunner.xul still loaded and active in order to define the
> window in which that text file is displayed?  You would dynamically modify the
> nodes under a menupopup node in the webrunner.xul to define the dock menu; the
> content document would not (and from a security standpoint probably should not)
> be involved.  The chrome DOM should be available to your chrome code even if a
> text document were loaded.  If you look at the Firefox portion of my patch, you
> will see that the "menu_mac_dockmenu" node is in an overlay over browser.xul.
> 
> You can expose a low-level interface to content from your webrunner.xul chrome.
>  This chrome code would simply make a new menuitem node using the chrome DOM
> (not the content DOM) and add it to the menupopup setup previously.  The code
> in my patch will notified of the change and update the dock menu accordingly.

Our key requirement is that content be able to modify system menus (e.g. the Dock menu). If I understand correctly, you are suggesting that we use a non-markup-based API like the one we have currently to enable content to create markup in the chrome DOM that is then used (via your markup-based API or some like it) to create the actual menu. Am I understanding correctly?

It seemed more natural for me for your API to use an underlying API that isn't based on markup, and to make that underlying ("low-level") API available where markup isn't appropriate (like our use case of content creating the menu). Creating the markup as an intermediate step seems unnecessary to me. Why do you prefer this?
(In reply to comment #31)
> 
> Our key requirement is that content be able to modify system menus (e.g. the
> Dock menu). If I understand correctly, you are suggesting that we use a
> non-markup-based API like the one we have currently to enable content to create
> markup in the chrome DOM that is then used (via your markup-based API or some
> like it) to create the actual menu. Am I understanding correctly?

Yes.  You can expose any non-markup-based API you like to Prism's content.  That's a Prism design decision and does not impact the toolkit/widget libraries one bit.

> It seemed more natural for me for your API to use an underlying API that isn't
> based on markup, and to make that underlying ("low-level") API available where
> markup isn't appropriate (like our use case of content creating the menu).
> Creating the markup as an intermediate step seems unnecessary to me. Why do you
> prefer this?

Several reasons why the natural method is actually a XUL-based API:

1.  IMHO, the best API for the widget-library code to expose is an API that will be used by the majority of XULRunner applications.  Most applications do not expose the Dock menu (or any other menu) to their content.  So the best API for these applications will let them define menus in exactly the same way they do now, i.e. XUL. Prism appears to be the exception, not the rule, here.

2.  The existing support for native menu bars in widget/src/cocoa expects XUL DOM nodes.  It required very little effort for me to utilize this existing code base to create a standalone native menu component. (Further evidence of what XULRunner applications typically do.)  Implementing the API you suggest will involve writing shims to create DOM nodes and add them to a DOM tree.  Writing these in C++ as part of nsNativeMenu.mm is much more involved than writing them in JavaScript.  Given Prism is the exception, these shims should be placed in Prism's chrome JavaScript.

3.  From a security standpoint, exposing the Dock menu directly to content seems to violate the principle that we should trust chrome, but should not trust content.  Does Prism impose any constraints on the number of menu entries that content can add to the Dock menu or system tray?  Even though the user may trust the web app that they have pointed Prism at, from a security standpoint, only chrome should be trusted.

4.  Does Prism have its own Dock menu entries in addition to the content menu entries?  If so, I would think you will need code in Prim's chrome JavaScript to merge Prism's entries (e.g., "reload web app") with the web app's entries.  The idea of having chrome code add its entries to a content-owned object seems to violate the security principle in #3 above.

5.  I actually see the proposed DOM-based API as the "low level" interface and your interface as the "high-level" interface since it simplifies that API for other callers in content (i.e., Facade design pattern).
Attachment #417814 - Attachment is obsolete: true
Attachment #421922 - Flags: review?(joshmoz)
Attachment #421923 - Flags: review?(joshmoz)
Attachment #421924 - Flags: review?(joshmoz)
(In reply to comment #29)
> (From update of attachment 417814 [details] [diff] [review])
> I think we should do this in 3 steps:
> 
> 1. Hash table fixes
> 2. Hoist DOM observer stuff into new class separate from nsMenuBarX
> 3. Implement dock menu
> 
> Please use NULL, nil, or nsnull instead of 0 where you can, when assigning to a
> pointer.

I split the patch as requested.  Each patch compiles and use of the menu bar still works.

Did you have an opinion on the naming of the new classes?
Comment on attachment 421922 [details] [diff] [review]
Patch 1.  Avoid deprecated hash table types.

>+  // put it in the table
>+  mCommandToMenuObjectTable.Put(mCurrentCommandID, inMenuItem);

This comment isn't necessary, it's obvious what is going on.

>-  nsPRUint32Key key(inCommandID);
>-  return reinterpret_cast<nsMenuItemX*>(mObserverTable.Get(&key));
>+  nsMenuItemX * result;
>+  if (!mCommandToMenuObjectTable.Get(inCommandID, &result))
>+    result = nsnull;
>+  return result;
> }

Why not just init result to nsnull here and avoid the "if" statement? Just use the same pattern you did in "nsMenuBarX::LookupContentChangeObserver".
(In reply to comment #39)
> 
> >-  nsPRUint32Key key(inCommandID);
> >-  return reinterpret_cast<nsMenuItemX*>(mObserverTable.Get(&key));
> >+  nsMenuItemX * result;
> >+  if (!mCommandToMenuObjectTable.Get(inCommandID, &result))
> >+    result = nsnull;
> >+  return result;
> > }
> 
> Why not just init result to nsnull here and avoid the "if" statement? Just use
> the same pattern you did in "nsMenuBarX::LookupContentChangeObserver".

Actually, looking at the definition of Get() in nsBaseHashtable.h, it does not update the out parameter if it returns PR_FALSE.  I actually need to update the other instance to conform to the if statement style.  Forgot to do so.
You should still init "result" to nsnull in case .Get() doesn't do it for you.
(In reply to comment #41)
> You should still init "result" to nsnull in case .Get() doesn't do it for you.

Except that if it returns PR_TRUE, it means it found an entry and wrote that value into the out parameter.  The code in content/base/src/nsContentUtils.cpp uses this style:

type value;
if (hashtable.Get(key, &value))
  return value;
else
  return nsnull;
Address Josh's comments. I used the pattern found elsewhere in the source code for accessing the nsDataHashtable<...>.
Attachment #421922 - Attachment is obsolete: true
Attachment #422054 - Flags: review?(joshmoz)
Attachment #421922 - Flags: review?(joshmoz)
Mirror changes made in Patch 1.1.
Attachment #421923 - Attachment is obsolete: true
Attachment #422055 - Flags: review?(joshmoz)
Attachment #421923 - Flags: review?(joshmoz)
If all windows are closed (and so no windows are displayed in the dock menu), do not add a menu separator.
Attachment #421925 - Attachment is obsolete: true
Attachment #422056 - Flags: review?(joshmoz)
Add the dock menu once via the DOM of the hidden window used by Firefox on Mac OS X. This fixes a bug where closing a window would leave the dock menu pointing at a non-existent document, as the dock menu was previously installed by the last active window.
Attachment #421926 - Attachment is obsolete: true
Helps to do "hg qrefresh" before uploading a patch ...
Attachment #422057 - Attachment is obsolete: true
Attachment #422054 - Flags: review?(joshmoz) → review+
(In reply to comment #32)
> Yes.  You can expose any non-markup-based API you like to Prism's content. 
> That's a Prism design decision and does not impact the toolkit/widget libraries
> one bit.

Sure. The goal was to harmonize our work if possible.

> Several reasons why the natural method is actually a XUL-based API:
> 
> 1.  IMHO, the best API for the widget-library code to expose is an API that
> will be used by the majority of XULRunner applications.  Most applications do
> not expose the Dock menu (or any other menu) to their content.  So the best API
> for these applications will let them define menus in exactly the same way they
> do now, i.e. XUL. Prism appears to be the exception, not the rule, here.

Let me be clear that I only ever suggest having two "levels" of API since I agree that most use cases will prefer a markup-based API. We are definitely the exception in that regard.

> 2.  The existing support for native menu bars in widget/src/cocoa expects XUL
> DOM nodes.  It required very little effort for me to utilize this existing code
> base to create a standalone native menu component. (Further evidence of what
> XULRunner applications typically do.)  Implementing the API you suggest will
> involve writing shims to create DOM nodes and add them to a DOM tree.  Writing
> these in C++ as part of nsNativeMenu.mm is much more involved than writing them
> in JavaScript.  Given Prism is the exception, these shims should be placed in
> Prism's chrome JavaScript.

I didn't realize there was existing code that expects a DOM. That is certainly a good argument for having DOM nodes be the "low-level" API.

> 3.  From a security standpoint, exposing the Dock menu directly to content
> seems to violate the principle that we should trust chrome, but should not
> trust content.  Does Prism impose any constraints on the number of menu entries
> that content can add to the Dock menu or system tray?  Even though the user may
> trust the web app that they have pointed Prism at, from a security standpoint,
> only chrome should be trusted.

We don't currently impose any constraints. Maybe we should, although as you say we do expect that web app running in Prism will be trusted, and we will probably be providing other privileges to it in the future (e.g. access to local file system) that a normal web app would not have.

> 4.  Does Prism have its own Dock menu entries in addition to the content menu
> entries?  If so, I would think you will need code in Prim's chrome JavaScript
> to merge Prism's entries (e.g., "reload web app") with the web app's entries. 
> The idea of having chrome code add its entries to a content-owned object seems
> to violate the security principle in #3 above.

Interesting point. How would you see the security risk from 3 and 4 manifesting itself?

> 5.  I actually see the proposed DOM-based API as the "low level" interface and
> your interface as the "high-level" interface since it simplifies that API for
> other callers in content (i.e., Facade design pattern).

Fair enough. If there is already code for native menus in the tree that uses DOM nodes as the API then it makes sense to use the same style API for other native menus like the OS X dock. I would love to use a markup-based API for Prism as well (in fact, that's what I had originally), but unfortunately there are cases where this simply isn't possible from content. But we can use our own API (maybe layered on yours, as you suggest, so that the user can choose which is appropriate).
Pushed hash table fixes to mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/1ae8b72947e8
Comment on attachment 422055 [details] [diff] [review]
Patch 2.1.  Hoist DOM mutation observer code into new superclass.

Can you rename "nsMenuOwnerX" to "nsMenuGroupOwnerX" to make it clear that there is not an owner for every individual menu? Also reflect this in member variable names like "mMenuOwner" (->"mMenuGroupOwner").
(In reply to comment #50)
> (From update of attachment 422055 [details] [diff] [review])
> Can you rename "nsMenuOwnerX" to "nsMenuGroupOwnerX" to make it clear that
> there is not an owner for every individual menu? Also reflect this in member
> variable names like "mMenuOwner" (->"mMenuGroupOwner").

Of course.  Do you have any other comments on the code?  I'd like to make the modifications all at once.
Renamed *MenuOwner* to *MenuGroupOwner* as per Josh's request.
Attachment #422055 - Attachment is obsolete: true
Attachment #422282 - Flags: review?(joshmoz)
Attachment #422055 - Flags: review?(joshmoz)
Rename *MenuOwner* to *MenuGroupOwner* as per Josh's request.
Attachment #421924 - Attachment is obsolete: true
Attachment #422283 - Flags: review?(joshmoz)
Attachment #421924 - Flags: review?(joshmoz)
(In reply to comment #48)

> > 4.  Does Prism have its own Dock menu entries in addition to the content menu
> > entries?  If so, I would think you will need code in Prim's chrome JavaScript
> > to merge Prism's entries (e.g., "reload web app") with the web app's entries. 
> > The idea of having chrome code add its entries to a content-owned object seems
> > to violate the security principle in #3 above.
> 
> Interesting point. How would you see the security risk from 3 and 4 manifesting
> itself?

I'm probably more concerned about denial of service (like the web app adds 50 menu items or something).  Probably not a problem since the user will have selected to use Prism to turn the web app into a desktop app.

However, the code in widget/src/cocoa/nsMenuItemX.mm marks the events it dispatches for menu commands as trusted.  I'm not familiar with the Mozilla security model and am unsure whether this means that content DOM nodes will get chrome privileges. (This has nothing to do with Prism of course.)

Also, part of me didn't like the thought of chrome passing its DOM nodes to a content-created instance of nsINativeMenu to add a menu item (not sure if this is the reality). More of my lack of familiarity with the Mozilla security model.

> > 5.  I actually see the proposed DOM-based API as the "low level" interface and
> > your interface as the "high-level" interface since it simplifies that API for
> > other callers in content (i.e., Facade design pattern).
> 
> Fair enough. If there is already code for native menus in the tree that uses
> DOM nodes as the API then it makes sense to use the same style API for other
> native menus like the OS X dock. I would love to use a markup-based API for
> Prism as well (in fact, that's what I had originally), but unfortunately there
> are cases where this simply isn't possible from content. But we can use our own
> API (maybe layered on yours, as you suggest, so that the user can choose which
> is appropriate).

The patches currently have a markup-based nsINativeMenu interface.  This will conflict with the nsINativeMenu interface defined in Prism.  Given that there may not be Windows-support at all for the markup-based nsINativeMenu interface, it may make sense to rename it to avoid the conflict.  Any suggestions?  (nsIMacNativeXULMenu?)
Comment on attachment 422282 [details] [diff] [review]
Patch 2.2.  Hoist DOM mutation observer code into new superclass.

I'm not sure that nsMenuGroupOwnerX should inherit nsMenuObjectX. The latter interface is for real menu objects, things that have native data for example, and the owner class doesn't fit that description. Seems like nsMenuBarX should inherit both interfaces separately. Is there a specific reason you have the inheritance set up this way?

I'm not finished commenting yet, no need to update the patch. Thanks for updating earlier though.
(In reply to comment #55)
> (From update of attachment 422282 [details] [diff] [review])
> I'm not sure that nsMenuGroupOwnerX should inherit nsMenuObjectX. The latter
> interface is for real menu objects, things that have native data for example,
> and the owner class doesn't fit that description. Seems like nsMenuBarX should
> inherit both interfaces separately. Is there a specific reason you have the
> inheritance set up this way?

I try to avoid multiple inheritance when I can (other than to use it for abstract interfaces).  nsMenuGroupOwnerX can be viewed either as an independent base class for any object that can own native menu objects or as a nsMenuObjectX that can own native menu objects.  In the latter case, it is of course only useful through its concrete subclasses (i.e., the menu bar and the standalone native menu), but the latter description is still a valid one.

That said, I don't particularly care whether or not nsMenuBarX and nsNativeMenu multiply inherit from nsMenuObjectX and nsMenuGroupX.  Your choice.
OK, lets have nsMenuBarX inherit nsMenuObjectX and nsMenuGroupOwnerX instead of having nsMenuGroupOwnerX inherit nsMenuObjectX. I want to keep nsMenuGroupOwnerX small and well-defined.

And on that note...

+  virtual nsresult InsertMenuAtIndex(nsMenuX* aMenu, PRUint32 aIndex);
+  virtual void RemoveMenuAtIndex(PRUint32 aIndex);

I don't think this stuff should be in nsMenuGroupOwnerX. We should simply pass on notification directly and let the real menu object classes handle that stuff. So for example - in "nsMenuGroupOwnerX::ContentRemoved" we should not be checking "if (aContainer == mContent)". The nsMenuBarX object can do that check when it gets a notification and respond appropriately. Same goes for "nsMenuGroupOwnerX::ContentInserted". Make sense?

If you can fix those two things I think we're good to go. Thanks again!
Attachment #422282 - Flags: review?(joshmoz) → review-
(In reply to comment #57)
> OK, lets have nsMenuBarX inherit nsMenuObjectX and nsMenuGroupOwnerX instead of
> having nsMenuGroupOwnerX inherit nsMenuObjectX. I want to keep
> nsMenuGroupOwnerX small and well-defined.

OK

> And on that note...
> 
> +  virtual nsresult InsertMenuAtIndex(nsMenuX* aMenu, PRUint32 aIndex);
> +  virtual void RemoveMenuAtIndex(PRUint32 aIndex);
> 
> I don't think this stuff should be in nsMenuGroupOwnerX. We should simply pass
> on notification directly and let the real menu object classes handle that
> stuff. So for example - in "nsMenuGroupOwnerX::ContentRemoved" we should not be
> checking "if (aContainer == mContent)". The nsMenuBarX object can do that check
> when it gets a notification and respond appropriately. Same goes for
> "nsMenuGroupOwnerX::ContentInserted". Make sense?

This would actually be a change in semantics, rather than a pure refactoring.  nsMenuBarX currently receives its notifications because it is the DOM mutation observer.  It does not register itself (with itself ...) using RegisterForContentChanges().

More to the point, nsMenuGroupOwner's implementations of ContentRemoved() and ContentInserted() -- the DOM mutation observer methods -- have been refactored out of nsMenuBarX with no changes.  I just made InsertMenuAtIndex() and RemoveMenuAtIndex() into template methods so that the unchanged nsMenuBarX implementations of those methods could be called by the code now in a base class.  (nsNativeMenu actually does not implement those methods.)

Maybe nsMenuBarX should register itself using RegisterForContentChanges()?  This would be a semantics change, however, which will need testing.
(In reply to comment #58)

> RemoveMenuAtIndex() into template methods so that the unchanged nsMenuBarX

in the design pattern sense, not the C++ template sense
I see what you're saying, let me ponder for a bit.
+  virtual nsresult InsertMenuAtIndex(nsMenuX* aMenu, PRUint32 aIndex);
+  virtual void RemoveMenuAtIndex(PRUint32 aIndex);

Lets leave this stuff the way it is in your patch, but make the above methods pure virtual so we can get rid of those dummy implementations.

So just change the nsMenuObjectX inheritance and make those methods pure virtual and I think we're good to go.
(In reply to comment #61)
> +  virtual nsresult InsertMenuAtIndex(nsMenuX* aMenu, PRUint32 aIndex);
> +  virtual void RemoveMenuAtIndex(PRUint32 aIndex);
> 
> Lets leave this stuff the way it is in your patch, but make the above methods
> pure virtual so we can get rid of those dummy implementations.
> 
> So just change the nsMenuObjectX inheritance and make those methods pure
> virtual and I think we're good to go.

OK.  I'll just have to add a dummy implementation to nsNativeMenu then since that is a concrete subclass.

Also, I'd like to rename it to nsINativeXULMenu or something Mac-specific like nsIMacNativeXULMenu.  Any preference?  (I don't see this API showing up on Windows any time soon for the system tray.)
(In reply to comment #54)
> I'm probably more concerned about denial of service (like the web app adds 50
> menu items or something).  Probably not a problem since the user will have
> selected to use Prism to turn the web app into a desktop app.
> 
> However, the code in widget/src/cocoa/nsMenuItemX.mm marks the events it
> dispatches for menu commands as trusted.  I'm not familiar with the Mozilla
> security model and am unsure whether this means that content DOM nodes will get
> chrome privileges. (This has nothing to do with Prism of course.)
> 
> Also, part of me didn't like the thought of chrome passing its DOM nodes to a
> content-created instance of nsINativeMenu to add a menu item (not sure if this
> is the reality). More of my lack of familiarity with the Mozilla security
> model.

Okay, I'll keep this in mind. We'll probably have more access to security-savvy resources (not really my specialty) as we look into folding Prism functionality into Firefox.

> The patches currently have a markup-based nsINativeMenu interface.  This will
> conflict with the nsINativeMenu interface defined in Prism.  Given that there
> may not be Windows-support at all for the markup-based nsINativeMenu interface,
> it may make sense to rename it to avoid the conflict.  Any suggestions? 
> (nsIMacNativeXULMenu?)

Your interface isn't Mac-specific, is it? I'd suggest you go with nsINativeMenu if the idea is for your interface to be the one that is most broadly used. I'll have to think about what to rename our interface to. Based on our prior discussion I am regretting that we moved away from using markup, so I'll give some more thought to this. Perhaps I'll come up with some clever way so that we can use markup even when there is no content page loaded. In that case we can simply use your interface which was the original goal.
(In reply to comment #62)
> (In reply to comment #61)
> > So just change the nsMenuObjectX inheritance and make those methods pure
> > virtual and I think we're good to go.
> 
> OK.  I'll just have to add a dummy implementation to nsNativeMenu then since
> that is a concrete subclass.

I didn't realize that nsNativeMenu doesn't need them. Never mind about pure virtual then.

> Also, I'd like to rename it to nsINativeXULMenu or something Mac-specific like
> nsIMacNativeXULMenu.  Any preference?  (I don't see this API showing up on
> Windows any time soon for the system tray.)

I want to avoid any name that sounds like it could apply to anything in the menu bar system, but that is going to be hard. Every menu in our system involves XUL and a native menu.

Since in most theoretical cases this is a menu that applies to the application maybe we should use nsIApplicationNativeMenu? Other options might be nsINativeSystemMenu or since the dock is the only use case you're imagining maybe we should just call it nsINativeDockMenu. I think I'd vote for nsINativeDockMenu. We can wait to decide on a more general name until another consumer comes along.
Attachment #422056 - Flags: review?(joshmoz)
(In reply to comment #63)
> > The patches currently have a markup-based nsINativeMenu interface.  This will
> > conflict with the nsINativeMenu interface defined in Prism.  Given that there
> > may not be Windows-support at all for the markup-based nsINativeMenu interface,
> > it may make sense to rename it to avoid the conflict.  Any suggestions? 
> > (nsIMacNativeXULMenu?)
> 
> Your interface isn't Mac-specific, is it? I'd suggest you go with nsINativeMenu
> if the idea is for your interface to be the one that is most broadly used. I'll
> have to think about what to rename our interface to. Based on our prior
> discussion I am regretting that we moved away from using markup, so I'll give
> some more thought to this. Perhaps I'll come up with some clever way so that we
> can use markup even when there is no content page loaded. In that case we can
> simply use your interface which was the original goal.

I finally understand what you were getting out when you said "no content page loaded."  Firefox solved this issue for Mac OS X by creating a hidden window which is always loaded.

See browser/base/content/{hiddenWindow.xul,macBrowserOverlay.xul,browser.js}.  In browser.js, look at nonBrowserWindowStartup().

The dock menu patch for Firefox adds the dock menu using the chrome DOM of the hidden window.

If you are okay with nsINativeMenu as the name, then I'll keep that name.  I didn't want to unnecessarily force you to change the name Prism was using.
(In reply to comment #64)
> I didn't realize that nsNativeMenu doesn't need them. Never mind about pure
> virtual then.

As it turns out, in NativeMenuItemTarget -menuItemHit, I rely on nsMenuGroupOwnerX deriving from nsMenuObjectX in order to determine whether a given nsMenuGroupOwnerX is a menu bar or not.  Your thoughts?
Comment on attachment 422282 [details] [diff] [review]
Patch 2.2.  Hoist DOM mutation observer code into new superclass.

OK, lets go with patch v2.2.

For v3.1 I think the only issue is the name of nsINativeXULMenu (see my comment #64) and things like eNativeMenuObjectType.
Attachment #422282 - Flags: review- → review+
(In reply to comment #67)
> (From update of attachment 422282 [details] [diff] [review])
> OK, lets go with patch v2.2.
> 
> For v3.1 I think the only issue is the name of nsINativeXULMenu (see my comment
> #64) and things like eNativeMenuObjectType.

From the choices in comment #64, I'd vote for nsINativeSystemMenu. 

Any thoughts on using the name "nsIStandaloneNativeMenu"?  (with standalone denoting the independence of this menu from any particular menu bar hierarchy)
(In reply to comment #65)
> I finally understand what you were getting out when you said "no content page
> loaded."  Firefox solved this issue for Mac OS X by creating a hidden window
> which is always loaded.
> 
> See browser/base/content/{hiddenWindow.xul,macBrowserOverlay.xul,browser.js}. 
> In browser.js, look at nonBrowserWindowStartup().
> 
> The dock menu patch for Firefox adds the dock menu using the chrome DOM of the
> hidden window.
> 
> If you are okay with nsINativeMenu as the name, then I'll keep that name.  I
> didn't want to unnecessarily force you to change the name Prism was using.

Yeah, I've played around with a hidden window in the past for other reasons (e.g. protocol handlers). Go ahead with nsINativeMenu and I'll see about migrating my stuff over once that lands.
pushed patch v2.2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/bf8f48413646
Comment on attachment 422283 [details] [diff] [review]
Patch 3.1.  Introduce nsINativeMenu and nsIMacDockSupport components.

Lets go with nsIStandaloneNativeMenu and rename the other things accordingly. Once I've reviewed the updated patch we'll need to get sr from roc since we'll be modifying widget/public.
Attachment #422283 - Flags: review?(joshmoz) → review-
I added a test case to Patch 3.x for nsIStandaloneNativeMenu based on the existing test case for native menus.  After adding the test, I discovered that the special case in the DOM observer code in nsMenuGroupX that treats the root element of a menu bar or standalone native menu differently than in its child elements was preventing the standalone native menu from updating when child nodes were added to it.

This patch removes that special case.  Josh suggested removing the special case previously, but the change was not made to preserve existing semantics.  Turns out it should have been made!
Attachment #423920 - Flags: review?(joshmoz)
Renamed nsINativeMenu to nsIStandaloneNativeMenu. Added chrome test case based on existing test case for Mac OS X native menus.

Note: I commented out one series of tests that checks whether menu nodes with no children can be added to a nsIStandaloneNativeMenu.  I don't believe this corner-case functionality will be needed for dock menus.  I can look into enabling it if wanted.
Attachment #422283 - Attachment is obsolete: true
Attachment #423921 - Flags: review?(joshmoz)
Update for Patch 3.2.
Attachment #422056 - Attachment is obsolete: true
Update for Patch 3.2.
Attachment #422058 - Attachment is obsolete: true
Attachment #423923 - Flags: ui-review?
Attachment #423923 - Flags: ui-review?
Comment on attachment 423920 [details] [diff] [review]
Patch 2A.0: Remove special case in DOM observer code for the root nsIContent.

>+  // make sure we unregister ourselves as a content observer
>+  UnregisterForContentChanges(mContent);

Comment here is unnecessary, it's pretty clear what we're doing.

>-#endif // nsMenuBarX_h_
>+#endif // nsMenuGroupOwner_h_

"nsMenuGroupOwnerX_h_" <- you forgot the X

No need to update the patch, just notes so I remember to fix that stuff when I land it. Thanks!
Attachment #423920 - Flags: review?(joshmoz) → review+
pushed Patch 2A.0 to mozilla-central, forgot to fix my own nits but I'll get it later

http://hg.mozilla.org/mozilla-central/rev/b1833293ce2e
Comment on attachment 423921 [details] [diff] [review]
Patch 3.2.  Introduce nsIStandaloneNativeMenu and nsIMacDockSupport components.

+NS_IMETHODIMP
+nsStandaloneNativeMenu::GetNativeMenu(void ** aVoidPointer)
+{
+  if (mMenu) {
+    *aVoidPointer = mMenu->NativeData();
+    [[(NSObject *)(*aVoidPointer) retain] autorelease];
+    return NS_OK;
+  }  else {
+    *aVoidPointer = nsnull;
+    return NS_ERROR_NOT_INITIALIZED;
+  }
+}

You don't need the else case, just end the method that way.

No need to update the patch for this, I can get it when I land (if I remember, I did a bad job last time)
Attachment #423921 - Flags: superreview?(roc)
Attachment #423921 - Flags: review?(joshmoz)
Attachment #423921 - Flags: review+
status2.0: --- → wanted
Flags: wanted-firefox3.5+
Does nsIStandaloneNativeMenu.idl need to be compiled in on Windows/Linux, so the reference to it in browser/base/content/browser.js is valid even if only Mac OS X implements it for now?
Attachment #423923 - Flags: ui-review?(faaborg)
Attachment #423922 - Flags: review?(gavin.sharp)
Attachment #423923 - Flags: review?(gavin.sharp)
I'm actually on 10.5 at the moment, if you attach a screen shot or confirm that the functionality is identical to safari, i can give you a quick ui-r+
(In reply to comment #80)
> I'm actually on 10.5 at the moment, if you attach a screen shot or confirm that
> the functionality is identical to safari, i can give you a quick ui-r+

Confirmed.  The only application-specific entry in Safari's dock menu is a "New Window" menu item.  This patch will match that functionality in Firefox.
Comment on attachment 423923 [details] [diff] [review]
Patch 5.3.  Firefox:  Add dock menu support.

great work getting this functionality added, and thanks for the quick clarification.
Attachment #423923 - Flags: ui-review?(faaborg) → ui-review+
As I said in some other bugs (about the application menu) using special IDs is not the way to mark special elements, please do not land that.  You could use some other attribute instead (say, type="mac-native-menu" macmenutype="dock").  It's unfortunate that new code like this is still accepted in mac widget.
In general, widget code that somehow alters the xul layout should get review for a xul peer (i.e. neil).
(In reply to comment #84)
> As I said in some other bugs (about the application menu) using special IDs is
> not the way to mark special elements, please do not land that.  You could use
> some other attribute instead (say, type="mac-native-menu" macmenutype="dock"). 
> It's unfortunate that new code like this is still accepted in mac widget.

While that is true for the IDs used by the native menu code in widget/src/cocoa when processing the Application menu, that is not the case here.  The "id" element is only used by the code added to browser.js to retrieve the menupopup element and pass it to the init() method of the nsIStandaloneNativeMenu instance.  The widget code itself never uses any special ID for the dock menu.  Indeed, one can change the ID to "this_is_not_a_special_ID" and it would still work.  The only connection between this ID and the special IDs is that I chose to go with the same naming scheme for naming consistency.  If this is the source of the confusion, it can be changed to something else if necessary.

(In reply to comment #85)
> In general, widget code that somehow alters the xul layout should get review
> for a xul peer (i.e. neil).

IMHO, none of these patches modifies the xul layout. The "popupset" element is being used to store an "standalone" menupopup element, which fits with the purpose set forth in the MDC (https://developer.mozilla.org/en/XUL_Tutorial/Popup_Menus) as menupopups declared this way only are displayed if referenced by another element.  Similarly, the menupopup for the dock menu will only be "displayed" if used to construct an instance of nsIStandaloneNativeMenu stored in the dockMenu property of the nsIMacDockSupport service.  Please suggest another way of accomplishing this if you believe this method to be the wrong way to do this.
Attachment #423921 - Flags: superreview?(roc) → superreview+
Comment on attachment 423921 [details] [diff] [review]
Patch 3.2.  Introduce nsIStandaloneNativeMenu and nsIMacDockSupport components.

Looks great. Thanks Tom!
pushed patch 3.2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/b21188a34531
Attachment #423922 - Flags: review?(gavin.sharp)
Comment on attachment 423922 [details] [diff] [review]
Patch 4.2.  Toolkit: Update dock menu.

Changed reviewer to sub-module owner for toolkit/xre.
Attachment #423922 - Flags: review?(benjamin)
Comment on attachment 423922 [details] [diff] [review]
Patch 4.2.  Toolkit: Update dock menu.

delegating to Josh
Attachment #423922 - Flags: review?(benjamin) → review?(joshmoz)
Comment on attachment 423922 [details] [diff] [review]
Patch 4.2.  Toolkit: Update dock menu.

+  rv = dockMenu->GetNativeMenu(reinterpret_cast<void **>(&nativeDockMenu));

I don't think there is any need for this to be a reinterpret cast, can you just make it a static cast?
(In reply to comment #91)
> (From update of attachment 423922 [details] [diff] [review])
> +  rv = dockMenu->GetNativeMenu(reinterpret_cast<void **>(&nativeDockMenu));
> 
> I don't think there is any need for this to be a reinterpret cast, can you just
> make it a static cast?

Triggers compilation error to use static_cast:

/Users/tdyas/Projects/Firefox/Firefox-trunk/toolkit/xre/MacApplicationDelegate.mm: In function ‘NSMenu* -[MacApplicationDelegate applicationDockMenu:](MacApplicationDelegate*, objc_selector*, NSApplication*)’:
/Users/tdyas/Projects/Firefox/Firefox-trunk/toolkit/xre/MacApplicationDelegate.mm:297: error: invalid static_cast from type ‘NSMenu**’ to type ‘void**’
Comment on attachment 423922 [details] [diff] [review]
Patch 4.2.  Toolkit: Update dock menu.

ok
Attachment #423922 - Flags: review?(joshmoz) → review+
pushed patch 4.2 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/4c9605dedc55
Comment on attachment 423923 [details] [diff] [review]
Patch 5.3.  Firefox:  Add dock menu support.

Does creating and setting a nsIStandaloneNativeMenu really need to happen for each non-browser window load? Couldn't we just do this once, in hiddenWindow.xul's onload handler or something?
(In reply to comment #95)
> (From update of attachment 423923 [details] [diff] [review])
> Does creating and setting a nsIStandaloneNativeMenu really need to happen for
> each non-browser window load? Couldn't we just do this once, in
> hiddenWindow.xul's onload handler or something?

You are correct. It just needs to happen once when the hidden window is loaded. I thought it only triggered once given that the code is inside an if statement conditioned on window.location.href == "chrome://browser/content/hiddenWindow.xul"?
(In reply to comment #95)
> (From update of attachment 423923 [details] [diff] [review])
> Does creating and setting a nsIStandaloneNativeMenu really need to happen for
> each non-browser window load? Couldn't we just do this once, in
> hiddenWindow.xul's onload handler or something?

I verified that the code is triggered only once.  nonBrowserWindowStartup is the load event handler for hiddenWindow.xul and the my code is within a conditional that runs that code only once when the hidden window is loaded.  Do you want me to split the dock menu code from the other hidden window initialization code in that function?

Here is the patch to browser.js with more context:

--- a/browser/base/content/browser.js
+++ b/browser/base/content/browser.js
@@ -1479,32 +1479,49 @@ function nonBrowserWindowStartup()
 
   // If no windows are active (i.e. we're the hidden window), disable the close, minimize
   // and zoom menu commands as well
   if (window.location.href == "chrome://browser/content/hiddenWindow.xul")
   ^^^^^^ This is why it only runs once.
   {
     var hiddenWindowDisabledItems = ['cmd_close', 'minimizeWindow', 'zoomWindow'];
     for (var id in hiddenWindowDisabledItems)
     {
       element = document.getElementById(hiddenWindowDisabledItems[id]);
       if (element)
         element.setAttribute("disabled", "true");
     }
 
     // also hide the window-list separator
     element = document.getElementById("sep-window-list");
     element.setAttribute("hidden", "true");
+
+    // setup the dock menu
+    let dockMenuElement = document.getElementById("menu_mac_dockmenu");
+    if (dockMenuElement != null) {
+      let nativeMenu = Cc["@mozilla.org/widget/standalonenativemenu;1"]
+                       .createInstance(Ci.nsIStandaloneNativeMenu);
+
+      try {
+        nativeMenu.init(dockMenuElement);
+
+        let dockSupport = Cc["@mozilla.org/widget/macdocksupport;1"]
+                          .getService(Ci.nsIMacDockSupport);
+        dockSupport.dockMenu = nativeMenu;
+      }
+      catch (e) {
+      }
+    }
   }
 
 
   setTimeout(nonBrowserWindowDelayedStartup, 0);
 }
 
 function nonBrowserWindowDelayedStartup()
 {
   // initialise the offline listener
   BrowserOffline.init();
   
   // Set up Sanitize Item
   initializeSanitizer();
 
   // initialize the private browsing UI
   gPrivateBrowsingUI.init();
(In reply to comment #97)
> (In reply to comment #95) 
> I verified that the code is triggered only once.  nonBrowserWindowStartup is
> the load event handler for hiddenWindow.xul and the my code is within a
> conditional that runs that code only once when the hidden window is loaded. 

Do this means that the menu item for a new window will be shown when on the same desktop all windows will be minimized in the dock or hidden (cmd-H) ?
(In reply to comment #98)
> Do this means that the menu item for a new window will be shown when on the
> same desktop all windows will be minimized in the dock or hidden (cmd-H) ?

Yes.
Duplicate of this bug: 545908
Duplicate of this bug: 545953
(In reply to comment #96)
> I thought it only triggered once given that the code is inside an if statement
> conditioned on window.location.href ==
> "chrome://browser/content/hiddenWindow.xul"?

Ah, sorry, I missed that. I wonder why that code isn't just directly in hiddenWindow.xul.

Seems like we should probably ifdef XP_MACOSX the code, to avoid the getElementById() call in the non-Mac case, rather than just relying on getElementById() failing. Why is the try/catch needed?

I think this probably looks OK, but would probably be best if you could get Mano to sign off, rather than me.
(In reply to comment #102)
> Ah, sorry, I missed that. I wonder why that code isn't just directly in
> hiddenWindow.xul.

I'm not aware of the history of that code. Might be a cleanup to be considered for the future.

> Seems like we should probably ifdef XP_MACOSX the code, to avoid the
> getElementById() call in the non-Mac case, rather than just relying on
> getElementById() failing. Why is the try/catch needed?

nonBrowserWindowStartup(), nonBrowserWindowDelayedStartup(), and nonBrowserWindowShutdown() are actually already contained in a #ifdef XP_MACOSX ... #endif block.

I included the try...catch block so that any failure to initialize the nsIStandaloneNativeMenu instance only results in there being no dock menu.

> I think this probably looks OK, but would probably be best if you could get
> Mano to sign off, rather than me.

Will do.
Comment on attachment 423923 [details] [diff] [review]
Patch 5.3.  Firefox:  Add dock menu support.

Change reviewer to mano as per Gavin's comment.
Attachment #423923 - Flags: review?(gavin.sharp) → review?(mano)
Comment on attachment 423923 [details] [diff] [review]
Patch 5.3.  Firefox:  Add dock menu support.

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js
>--- a/browser/base/content/browser.js
>+++ b/browser/base/content/browser.js
>@@ -1488,6 +1488,23 @@
>     // also hide the window-list separator
>     element = document.getElementById("sep-window-list");
>     element.setAttribute("hidden", "true");
>+
>+    // setup the dock menu

nit: "Setup" and end with period.
 
>diff --git a/browser/base/content/macBrowserOverlay.xul b/browser/base/content/macBrowserOverlay.xul
>--- a/browser/base/content/macBrowserOverlay.xul
>+++ b/browser/base/content/macBrowserOverlay.xul
>@@ -73,4 +73,11 @@
> # hiddenWindow.xul.
> #include browser-menubar.inc
> 
>+# Dock menu

Use a normal |<!--| comment here.

r=mano.
Attachment #423923 - Flags: review?(mano) → review+
Revised as per mano's comments.  This should be ready for check-in.
Attachment #423923 - Attachment is obsolete: true
pushed patch 5.4 to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/4aec85d30806

Thanks Tom!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Summary: [10.5] Add "New window" entry in Dock Menu to better integrate with Spaces → Dock menu support for Mac OS X (add "New Window" menu item)
I think that should technically be "Set up" :)
Duplicate of this bug: 287954
Blocks: 538376
Flags: in-litmus?
I'm the (happy) reporter of the bug and I can confirm that the latest nightly is working exactly like expected. I set the bug as VERIFIED. (I hope I'm allowed to do it, if not, sorry)

I would like to thank everybody who work towards this and it was really fun (and interesting) to watch the work on it through these two years.

Thank you all!
Status: RESOLVED → VERIFIED
https://litmus.mozilla.org/show_test.cgi?id=11506 added to the Snow Leopard suite in Litmus.
Flags: in-litmus? → in-litmus+
Target Milestone: --- → Firefox 3.7a2
Blocks: 548500
What do I need to do to see this "New Window" menu item? I've tried it with the newest en-US Trunk nightly, with the newest german Nightly and with the MozillaDeveloperPreview 3.7 Alpha 2. But I don't see the "New Window" menu item. Does this only work if the OS language version is the same as the FF language version? Or do I have to do somethig to see it?
(In reply to comment #112)
> What do I need to do to see this "New Window" menu item? I've tried it with the
> newest en-US Trunk nightly, with the newest german Nightly and with the
> MozillaDeveloperPreview 3.7 Alpha 2. But I don't see the "New Window" menu
> item. Does this only work if the OS language version is the same as the FF
> language version? Or do I have to do somethig to see it?

It doesn't appear when you right-click the Firefox dock icon?
(In reply to comment #113)
> (In reply to comment #112)
> > What do I need to do to see this "New Window" menu item? I've tried it with the
> > newest en-US Trunk nightly, with the newest german Nightly and with the
> > MozillaDeveloperPreview 3.7 Alpha 2. But I don't see the "New Window" menu
> > item. Does this only work if the OS language version is the same as the FF
> > language version? Or do I have to do somethig to see it?
> 
> It doesn't appear when you right-click the Firefox dock icon?
Nope. 

But I've found my mistake, I didn't know that you have to launch the application first to get the new menu item. I've right-clicked on the closed dock icon. Sorry, my fault.
Blocks: 566063
I'm using 3.7a5 and have one question: why does the new window open in the space where the last active Firefox window resides? Shouldn't it open in the space I click the 'new window' dock menu option in?
(In reply to comment #115)
> I'm using 3.7a5 and have one question: why does the new window open in the
> space where the last active Firefox window resides? Shouldn't it open in the
> space I click the 'new window' dock menu option in?

If that is Safari's behavior, then we should probably open another bug to deal with that issue.
It's not just Safari's behavior, it's all Mac applications.  An application that doesn't behave this way is considered "behaving badly".
Blocks: 574229
I opened bug 574229 for the Spaces issues.
Duplicate of this bug: 596246
Blocks: 389614
No longer blocks: FF2SM
You need to log in before you can comment on or make changes to this bug.