393336 - Don't do ARGB->RGBA translation in nsClipboard.mm

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Colin Barrett [:cbarrett]]

Now that we don't support 10.3, we should get rid of the ARGB->RGBA code in nsClipboard.mm. Instead we should pass bitmapFormat:NSAlphaFirstBitmapFormat as one of the arguments to initWithBitmapDataPlanes...

http://mxr.mozilla.org/seamonkey/source/widget/src/cocoa/nsClipboard.mm#351

Comment 1

['Ili Butterfield]

Attachment: fix

Proposed fix.

Gets rid of all the reorderedData code and just calls initWithBitmapDataPlanes with the bitmapFormat argument.

Comment 2

[Colin Barrett [:cbarrett]]

Comment on attachment 278033 [details] [diff] [review]
fix

>-      planes[0] = (PRUint8*)reorderedData;
>+      planes[0] = image->GetBits();
>       planes[1] = nsnull;

>                                                                              isPlanar:NO

Since the data isn't planar, we don't need a null terminating plane either.

Other than that looks fine.

Comment 3

['Ili Butterfield]

Attachment: fix v2

New fix incorporating the change in the above comment as well.

Comment 4

[Colin Barrett [:cbarrett]]

Ergh, when doing pre-checkin sanity tests, I noticed that on my little endian machine, this doesn't work right. Seems NSBitmapImageFormat doesn't respect the native architecture and forces big-endian? Ugh.

Comment 5

[Colin Barrett [:cbarrett]]

Comment on attachment 278110 [details] [diff] [review]
fix v2

marking the patch r- so nobody drive-by checkins it.

Comment 6

['Ili Butterfield]

Attachment: fix v3

New fix that copies the image data directly into the bitmapData of the NSBitmapImageRep. Works on my PPC, and hopefully it'll work on a little-endian Intel machine too.

Comment 7

['Ili Butterfield]

Attachment: fix v4

This one actually works on one PPC machine and one Intel machine.

I chucked NSBitmapRep entirely and instead use Quartz functions to convert the image to TIFF for the pasteboard. CGImageCreate takes endianness into account, solving the architecture problem.

Comment 8

[Colin Barrett [:cbarrett]]

Comment on attachment 278191 [details] [diff] [review]
fix v4

looks good, tested it here, seems to work fine. Tried loading large images from NASA (5MB or so), no noticeable beachballing.

Comment 9

[Josh Aas]

Comment on attachment 278191 [details] [diff] [review]
fix v4

I want to look too before this lands, but go ahead with sr.

Comment 10

[Colin Barrett [:cbarrett]]

ah sure, that's fine. Probably should have requested r from you anyway, given that I'm not an actual peer.

Comment 11

[Josh Aas]

Comment on attachment 278191 [details] [diff] [review]
fix v4

+      [pasteboardOutputDict setObject:(NSMutableData *)tiffData forKey:NSTIFFPboardType];

No space between "NSMutableData" and the "*". Just fix that on checkin.

Comment 12

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 278191 [details] [diff] [review]
fix v4

sr=pink

Comment 13

[Josh Aas]

landed on trunk, thanks 'Ili!