Closed Bug 394282 Opened 15 years ago Closed 15 years ago

remove unnecessary docshell usage from cocoa menu impl


(Core :: Widget: Cocoa, defect)

Not set





(Reporter: jaas, Assigned: jaas)



(1 file, 2 obsolete files)

Attached patch fix v1.0 (obsolete) — Splinter Review
We can remove the docshell member variable from nsMenuItemX. This actually saves quite a few words of memory because we create a lot of those objects.
Attachment #278921 - Flags: review?(cbarrett)
Attachment #278921 - Flags: review?(cbarrett) → review+
Summary: remove unused member variable from nsMenuItemX → remove unnecessary docshell usage from cocoa menu impl
Attached patch fix v2.0 (obsolete) — Splinter Review
Turns out there is a ton of docshell usage we can get rid of, saving us a lot of time and memory. This completely gets rid of all docshell usage in nsMenuX and nsMenuItemX plus some in nsMenuBarX. There is a bunch of comment/whitespace cleanup in the nsMenuBarX changes, I don't want to redo that later so it is in this patch. It is easy to glance over.
Attachment #278921 - Attachment is obsolete: true
Attachment #278924 - Flags: review?(cbarrett)
Comment on attachment 278924 [details] [diff] [review]
fix v2.0

It would be nice if we could get rid of all references to docshell all together, but if not that's OK.
Attachment #278924 - Flags: review?(cbarrett) → review+
We should be able to get rid of most of it, but that would be a more invasive change. We'd have to switch to some other gecko APIs in nsMenuBarX and we'd have to modify nsIMenuBar, nsIMenu, and nsIMenuItem APIs. We can do it for Mozilla 2, hopefully when we decomtaminate most of it at the same time.
Attachment #278924 - Flags: superreview?(mikepinkerton)
Attachment #278924 - Flags: superreview?(mikepinkerton) → superreview?(bzbarsky)
Comment on attachment 278924 [details] [diff] [review]
fix v2.0

Why are those #includes moving into the headers?  I'd think you would either remove them altogether or keep them in the .cpp.

Also, it would be nice to have separate patches for functionality changes and stylistic cleanup, in general.  Combining the two makes it a pain to review...
The reason for moving the includes is that we use those types in the header but we don't include the headers for them.
How did it compile before?

In any case, I see no reason most of these can't be forward-decls instead of includes.  Certainly nsMenuBarX.h can; I didn't look at the others.

Unnecessary includes make compilation slow. Let's avoid them.  :)
Attached patch fix v2.1Splinter Review
Ah, thanks. Fixed.
Attachment #278924 - Attachment is obsolete: true
Attachment #278974 - Flags: superreview?(bzbarsky)
Attachment #278924 - Flags: superreview?(bzbarsky)
Attachment #278974 - Flags: review+
Comment on attachment 278974 [details] [diff] [review]
fix v2.1

Attachment #278974 - Flags: superreview?(bzbarsky) → superreview+
Attachment #278974 - Flags: 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.