remove nsIMenuListener interface

RESOLVED FIXED

Status

()

Core
Widget
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
Created attachment 285339 [details] [diff] [review]
fix v1.0

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

Comment 1

11 years ago
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
(Assignee)

Comment 2

11 years ago
Created attachment 285381 [details] [diff] [review]
fix v1.1
Attachment #285339 - Attachment is obsolete: true
Attachment #285381 - Flags: review?(cbarrett)
(Assignee)

Updated

11 years ago
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.
(Assignee)

Comment 4

11 years ago
Created attachment 285512 [details] [diff] [review]
fix v1.2

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 5

11 years ago
Comment on attachment 285512 [details] [diff] [review]
fix v1.2

r=smorgan
Attachment #285512 - Flags: review?(stuart.morgan) → review+
(Assignee)

Updated

11 years ago
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)?
(Assignee)

Comment 7

11 years ago
Created attachment 285843 [details] [diff] [review]
fix v1.3

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+
(Assignee)

Updated

11 years ago
Attachment #285843 - Flags: approval1.9?
(Assignee)

Comment 8

11 years ago
Surprisingly, this seems to knock our native menu loading time from 79ms to 72ms on my machine using Shark, which is almost 10%.

Updated

11 years ago
Attachment #285843 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 9

11 years ago
landed on trunk
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.