Closed
Bug 544704
Opened 14 years ago
Closed 14 years ago
Remove duplicate image conversion code from nsMenuItemIconX.mm and nsClipboard.mm
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: lordpixel, Assigned: lordpixel)
References
Details
Attachments
(1 file, 2 obsolete files)
10.64 KB,
patch
|
jaas
:
review+
|
Details | Diff | Splinter Review |
nsMenuItemIconX and nsClipboard both contain code to convert from imgIContainer to CGImageRef. As a part of bug https://bugzilla.mozilla.org/show_bug.cgi?id=286304 library routines have been added to nsCocoaUtils to do this conversion. This bug is about converting over the two classes above so all 3 places share the same conversion code.
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 years ago
|
||
nsClipboard patch is straightforward. nsMenuItemIconX patch takes advantage of some APIs added in 10.4 to cut down the amount of custom code required. Fortunately we are now able to avoid messing around with the pixels directly at all.
Attachment #425724 -
Flags: review?(joshmoz)
Comment on attachment 425724 [details] [diff] [review] Remove image conversion code + CGImageRef iconImage = CGBitmapContextCreateImage(bitmapContext); It's hard to see in the diff but there are two early returns after this code where "iconImage" will leak. This check appears twice with your patch in "nsMenuItemIconX::OnStopFrame" and you only need the first one: if (!mNativeMenuItem) return NS_ERROR_FAILURE; The second one, if triggered, will actually cause "newImage" and "iconImage" to leak. Just remove it to avoid these leaks. CGImageCreateWithImageInRect and CGBitmapContextCreateImage are missing the "::" prefix in at least one place.
Attachment #425724 -
Flags: review?(joshmoz) → review-
Assignee | ||
Comment 3•14 years ago
|
||
Fix problems Josh spotted.
Attachment #425724 -
Attachment is obsolete: true
Attachment #426831 -
Flags: review?(joshmoz)
Comment on attachment 426831 [details] [diff] [review] Revised patch addressing Josh's comments Thanks
Attachment #426831 -
Flags: review?(joshmoz) → review+
Assignee | ||
Comment 5•14 years ago
|
||
Unfortunately I didn't find enough time to check this in and now it collided with bug 548712. Here's a reworked patch which merges the two changes. Apologies for making you review it again.
Attachment #430901 -
Flags: review?(joshmoz)
Assignee | ||
Updated•14 years ago
|
Attachment #426831 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Attachment #430901 -
Attachment description: Merge patch with changes from big 548712 → Merge patch with changes from bug 548712
Waiting to review this until bug 286304 lands...
Comment on attachment 430901 [details] [diff] [review] Merge patch with changes from bug 548712 With this patch you can also kill off PRAllocCGFree in nsMenuItemIconX.mm.
Attachment #430901 -
Flags: review?(joshmoz) → review+
pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/59e5e15f723f Forgot to take my own review comment into account, will fix in a later push. Thanks Andrew!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
removed PRAllocCGFree http://hg.mozilla.org/mozilla-central/rev/b4716ecdf427
Comment 10•14 years ago
|
||
I suspect this caused the crashes seen in bug 552616. Searching Socorro for "CGAccessSessionGetBytes" crashes in the last 3 weeks, I get 88 crashes, all but 2 of which started with the 20100314141333 nightly (the first one after this landed).
Depends on: 552616
You need to log in
before you can comment on or make changes to this bug.
Description
•