391288 - Clean up lifetime of nsMenuX's NSMenuItem* mNativeMenuItem

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[jag (Peter Annema)]

Attachment: Clarify lifetime of nsMenuX's mNativeMenuItem

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?

Comment 1

[Stan Shebs]

I've been studying the patch already, it passes my own testing, so I'll take unless someone else wants it.

Comment 2

[jag (Peter Annema)]

Attachment: Same as above but -pu10

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.

Comment 3

[jag (Peter Annema)]

Comment on attachment 275693 [details] [diff] [review]
Same as above but -pu10

You win :-)

Comment 4

[Stan Shebs]

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.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

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 :(

Comment 6

[jag (Peter Annema)]

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".

Comment 7

[Josh Aas]

landed on trunk

Comment 8

[jag (Peter Annema)]

Heh, do I get to check anything in these days?

Thanks Josh :-)

Comment 9

[Josh Aas]

Attachment: enabled confusion fix, v1.0

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.

Comment 10

[Josh Aas]

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.