Closed Bug 391288 Opened 15 years ago Closed 15 years ago

Clean up lifetime of nsMenuX's NSMenuItem* mNativeMenuItem

Categories

(Core :: Widget: Cocoa, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jag+mozilla, Assigned: jag+mozilla)

References

Details

Attachments

(2 files, 1 obsolete file)

See bug 390707 for what led to this. I took the last patch and moved some more stuff around. Anyone want to volunteer to look at my changes and see if they make sense?
I've been studying the patch already, it passes my own testing, so I'll take unless someone else wants it.
So this patch makes nsMenuX own its NSMenuItem* mNativeMenuItem, like nsMenuItemX already did. I moved most of the nsMenuX and nsMenuItemX setup code into their respective ::Create() methods, to make that work. In figuring this code out I saw some low-hanging-fruit-style clean-up, so I'm throwing that in for free.
Attachment #275691 - Attachment is obsolete: true
Comment on attachment 275693 [details] [diff] [review]
Same as above but -pu10

You win :-)
Attachment #275693 - Flags: review?(stanshebs)
Attachment #275693 - Flags: review+
Comment on attachment 275693 [details] [diff] [review]
Same as above but -pu10

Yes, this is a big improvement. Every nsMenuX has its native menu item assigned on creation, so lifetime issues go away entirely.
Attachment #275693 - Flags: review?(stanshebs) → review+
Attachment #275693 - Flags: superreview?(vladimir)
Comment on attachment 275693 [details] [diff] [review]
Same as above but -pu10

Looks ok, though ugh on mixing cleanups and functional changes in one patch :(
Attachment #275693 - Flags: superreview?(vladimir) → superreview+
Comment on attachment 275693 [details] [diff] [review]
Same as above but -pu10

Requesting a1.9+

cbarrett says this will make his patch for bug 382194 "much much much easier".
Attachment #275693 - Flags: approval1.9?
Blocks: 382194
Attachment #275693 - Flags: approval1.9? → approval1.9+
landed on trunk
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Heh, do I get to check anything in these days?

Thanks Josh :-)
Clarify enabled usage, remove jag's comment about whether we can us mIsEnabled. We can't use mIsEnabled there because that is the call that sets it up.
Attachment #281232 - Flags: review?(cbarrett)
Attachment #281232 - Flags: review?(cbarrett) → review+
Attachment #281232 - Flags: approval1.9+
Comment on attachment 281232 [details] [diff] [review]
enabled confusion fix, v1.0

meh, I didn't mean to actually change any code, just remove the comment. fixed on checkin.
You need to log in before you can comment on or make changes to this bug.