Closed Bug 400259 Opened 15 years ago Closed 15 years ago

remove nsIMenuListener interface


(Core :: Widget, defect)

Not set





(Reporter: jaas, Assigned: jaas)



(1 file, 3 obsolete files)

Attached patch fix v1.0 (obsolete) — Splinter Review
We can probably safely remove the nsIMenuListener interface now, which I did in some of my spare time recently.

This saves 4 bytes per nsIWidget on all platforms. Saves 4 bytes per menu item on Mac OS X (we can easily have many hundreds of menu items in existence with a few Firefox windows open). Saves 4 bytes per menu on Mac OS X. We save a small amount of memory in overall code size on all platforms.

Fewer expensive QI calls in critical places. More direct typing increases code clarity. Removes a lot of unnecessary code, including many method implementations that only exist to satisfy common interface requirements. It'll be much easier to decomtaminate our Mac OS X menu code in the future with this (exploring menu impl decom is what led me to doing this in the first place).
Note - I didn't request review yet on purpose. I am pretty sure the current patch violates some rules about modifying interfaces, it doesn't bump the interface ids for the interfaces it modifies, and I haven't finished my own review yet.
OS: Mac OS X → All
Hardware: PC → All
Attached patch fix v1.1 (obsolete) — Splinter Review
Attachment #285339 - Attachment is obsolete: true
Attachment #285381 - Flags: review?(cbarrett)
Attachment #285381 - Flags: review?(cbarrett) → review?(stuart.morgan)
Sorry I didn't get to this yesterday, Josh. 

I took a look through the patch anyway, and noticed that with a number of the nsIMenuListener methods, you changed the return type from nsEventStatus to NS_IMETHOD but didn't change the actual value that is getting returned. Example:

> -nsEventStatus nsMenuX::MenuDestruct(const nsMenuEvent & aMenuEvent)
> +NS_IMETHODIMP nsMenuX::MenuDestruct(const nsMenuEvent & aMenuEvent)
>  {
>    ...
>    return nsEventStatus_eIgnore;
>  }

Not sure why the compiler yell didn't at you for that.
Attached patch fix v1.2 (obsolete) — Splinter Review
Fixes the return values and also completely removes the nsIMenuListener interface. Turns out we can drop AddMenuListener from nsIWidget.
Attachment #285381 - Attachment is obsolete: true
Attachment #285512 - Flags: review?(stuart.morgan)
Attachment #285381 - Flags: review?(stuart.morgan)
Comment on attachment 285512 [details] [diff] [review]
fix v1.2

Attachment #285512 - Flags: review?(stuart.morgan) → review+
Attachment #285512 - Flags: superreview?(roc)
You're not checking the results of the new methods in nsIMenu. Why not make them void (or MenuSelected could return the event status directly)?
Attached patch fix v1.3Splinter Review
Changed interfaces methods that don't need a return status to void, return nsEventStatus directly otherwise.
Attachment #285512 - Attachment is obsolete: true
Attachment #285843 - Flags: superreview?(roc)
Attachment #285512 - Flags: superreview?(roc)
Attachment #285843 - Flags: superreview?(roc) → superreview+
Attachment #285843 - Flags: approval1.9?
Surprisingly, this seems to knock our native menu loading time from 79ms to 72ms on my machine using Shark, which is almost 10%.
Attachment #285843 - Flags: approval1.9? → approval1.9+
landed on trunk
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.