This interface can just be combined into nsIRollupListener.

Comment on attachment 569039 [details] [diff] [review] patch > layout/build/nsLayoutModule.cpp >- { "@mozilla.org/xul/xul-popup-manager;1", &kNS_XULPOPUPMANAGER_CID }, This doesn't seem necessary for removing nsIMenuRollup. Do you know "xul-popup-manager" isn't used by any add-on? Through http://www.google.com/codesearch I found one that at least makes a reference to it: https://github.com/nightwing/foximirror Please get sr+ from the module owner on this change. > widget/public/nsIRollupListener.h >- NS_IMETHOD Rollup(PRUint32 aCount, nsIContent **aContent) = 0; >+ virtual nsIContent* Rollup(PRUint32 aCount, bool aGetLastRolledUp = false) = 0; Maybe it's just me, but... With the old signature I would assume that aContent is AddRef-ed. With the new signature I would assume that the return value is NOT AddRef-ed. Would it possible to return already_AddRefed<nsIContent> ? If not, I would prefer to return a non-addref pointer and let the consumer addref, if it needs it. >+ * If aGetLastRolledUp is true, then return the last rolled up popup. nsComboboxControlFrame::Rollup always returns null though... (not sure how to document that...) + /* + * Retrieve the widgets for open menus are store them in the array + * aWidgetChain. The number of menus of the same type should be returned, + * for example, if a context menu is open, return only the number of menus + * that are part of the context menu chain. This allows closing up only + * those menus in different situations. + */ + virtual PRUint32 GetSubmenuWidgetChain(nsTArray<nsIWidget*> *aWidgetChain) = 0; s/are store/and store/ The comment is slightly confusing, can you make it clearer that the returned PRUint32 is exactly the number of widgets appended to aWidgetChain by this call. > layout/xul/base/public/nsXULPopupManager.h >- virtual PRUint32 GetSubmenuWidgetChain(nsTArray<nsIWidget*> *aWidgetChain); > [...] >+ PRUint32 GetSubmenuWidgetChain(nsTArray<nsIWidget*> *aWidgetChain); Why this change? It still overrides nsIRollupListener::GetSubmenuWidgetChain (?) so I think it should be grouped with the other nsIRollupListener methods (with an explicit 'virtual').

(In reply to Mats Palmgren [:mats] from comment #1) > This doesn't seem necessary for removing nsIMenuRollup. > Do you know "xul-popup-manager" isn't used by any add-on? > Through http://www.google.com/codesearch I found one that at least > makes a reference to it: https://github.com/nightwing/foximirror > The popup manager only exists as a component due to needing to access it from outside of layout. That requirement no longer exists. The popup manager isn't scriptable, nor should it be. I can't see any reference to the popup manager in the code you linked to.

> The popup manager only exists as a component due to needing to access it > from outside of layout. That requirement no longer exists. Ok, I'm just recommending you query AMO for example to see what authors ACTUALLY do with this service, rather than say they SHOULDn't have used it. Often enough, they tend to disagree with us... ;-) Don't get me wrong, I'm all for removing it and the code looks fine to me from a review point, but I feel I don't have the authority to say it's ok to remove it -- on the contrary, I think it's my obligation as a reviewer to point out that I think it's something the module owner should approve. > The popup manager isn't scriptable The interfaces it implements are scriptable AFAICT. I entered in Error Console: Components.classes["@mozilla.org/xul/xul-popup-manager;1"].getService().QueryInterface(Components.interfaces.nsIDOMEventListener).handleEvent(null) and my breakpoint in nsXULPopupManager::HandleEvent was hit... > I can't see any reference to the popup manager in the code you linked to. http://www.google.com/codesearch#dcUp1IdzD9U/chrome/idleMirror/interfaceData.json&q=xul-popup-manager%20-nsLayoutModule&type=cs

Comment on attachment 569039 [details] [diff] [review] patch Review of attachment 569039 [details] [diff] [review]: ----------------------------------------------------------------- Mats is right, we need to check AMO to see if there is any addon compat issue here.

Searching around for popup manager usage didn't turn anything up. It's not clear what compatibility issue I'd be looking for though.

Comment on attachment 571074 [details] [diff] [review] Updated patch > widget/src/windows/nsWindow.cpp > // only need to deal with the last rollup for left mouse down events. >- sRollupListener->Rollup(popupsToRollup, inMsg == WM_LBUTTONDOWN ? &mLastRollup : nsnull); >+ mLastRollup = sRollupListener->Rollup(popupsToRollup, inMsg == WM_LBUTTONDOWN); >+ NS_IF_ADDREF(mLastRollup); mLastRollup must be null before the assignment for this code to be correct. I think we should assert that. (also in widget/src/os2/nsWindow.cpp) FYI, the addref above didn't compile for me on XP - something about nsIContent being an unknown type... please check this before landing. r=mats