Closed
Bug 696745
Opened 13 years ago
Closed 13 years ago
Remove nsIMenuRollup
Categories
(Core :: XUL, defect)
Core
XUL
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: enndeakin, Assigned: enndeakin)
References
Details
Attachments
(1 file, 1 obsolete file)
72.81 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
This interface can just be combined into nsIRollupListener.
Attachment #569039 -
Flags: review?(matspal)
Comment 1•13 years ago
|
||
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').
Assignee | ||
Comment 2•13 years ago
|
||
(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•13 years ago
|
||
> 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
Assignee | ||
Updated•13 years ago
|
Attachment #569039 -
Flags: superreview?(roc)
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.
Attachment #569039 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 5•13 years ago
|
||
Searching around for popup manager usage didn't turn anything up. It's not clear what compatibility issue I'd be looking for though.
Attachment #569039 -
Attachment is obsolete: true
Attachment #569039 -
Flags: review?(matspal)
Attachment #571074 -
Flags: review?(matspal)
Comment 6•13 years ago
|
||
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
Attachment #571074 -
Flags: review?(matspal) → review+
Comment 7•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Updated•6 years ago
|
Component: XP Toolkit/Widgets: Menus → XUL
You need to log in
before you can comment on or make changes to this bug.
Description
•