696745 - Remove nsIMenuRollup

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[Neil Deakin]

Attachment: patch

This interface can just be combined into nsIRollupListener.

Comment 1

[Mats Palmgren (inactive)]

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').

Comment 2

[Neil Deakin]

(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.

Comment 3

[Mats Palmgren (inactive)]

> 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 4

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

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.

Comment 5

[Neil Deakin]

Attachment: Updated patch

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 6

[Mats Palmgren (inactive)]

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

Comment 7

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/e3461cc0b174