Last Comment Bug 696745 - Remove nsIMenuRollup
: Remove nsIMenuRollup
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: Menus (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla11
Assigned To: Neil Deakin
:
Mentors:
Depends on: 701061
Blocks: 699538
  Show dependency treegraph
 
Reported: 2011-10-24 06:12 PDT by Neil Deakin
Modified: 2011-11-09 12:39 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (72.75 KB, patch)
2011-10-24 06:12 PDT, Neil Deakin
roc: superreview+
Details | Diff | Splinter Review
Updated patch (72.81 KB, patch)
2011-11-01 11:31 PDT, Neil Deakin
mats: review+
Details | Diff | Splinter Review

Description Neil Deakin 2011-10-24 06:12:00 PDT
Created attachment 569039 [details] [diff] [review]
patch

This interface can just be combined into nsIRollupListener.
Comment 1 Mats Palmgren (:mats) 2011-10-24 15:15:12 PDT
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 2011-10-24 19:49:22 PDT
(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 (:mats) 2011-10-25 10:41:06 PDT
> 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) 2011-10-25 11:31:45 PDT
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 2011-11-01 11:31:57 PDT
Created attachment 571074 [details] [diff] [review]
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 (:mats) 2011-11-05 15:11:28 PDT
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] 2011-11-09 05:27:43 PST
https://hg.mozilla.org/mozilla-central/rev/e3461cc0b174

Note You need to log in before you can comment on or make changes to this bug.