286897 - XBM cursor hotspots

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Aaron Kaluszka]

Now that bug 38447 is fixed, hotspot data from XBM cursors should be read.  I
will upload the patch as soon as bug 38447 comment 163 is resolved.

Comment 1

[Aaron Kaluszka]

Attachment: patch

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 178000 [details] [diff] [review]
patch

hm... so if necko gives us the data in just the right blocks, we won't find the
hotspot... I guess the code doesn't deal with that anyway (size might be
truncated in the middle).

I wonder if it might've been better if this decoder were non-incremental? xbm
images are usually not large, I guess. anyway, not a topic for this bug.

-	 PRBool hiByte = PR_TRUE;
+	 PRPackedBool hiByte = PR_TRUE;

why bother? this is a stack variable. no need to save space.

-    else
-	 return NS_ERROR_FAILURE;

hmm, this is bug 251446

r=biesi if you leave the highByte as PRBool.

Comment 3

[Aaron Kaluszka]

Attachment: patch

This patch addresses the review comments.  In particular, the problem regarding
grabbing the size/hotspot data incrementally is fixed.

Comment 4

[tor]

Comment on attachment 178011 [details] [diff] [review]
patch

>Index: mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp
>-    else
>-        return NS_ERROR_FAILURE;
>-    
>+

As biesi mentioned this is bug 251446 and should be left there.

>Index: mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.h
>+    PRPackedBool mIsCursor;
>+    PRPackedBool mIsX10; // X10 flavor XBM?
>     PRUint8* mRow; // Hold the decoded row
>     PRUint8* mAlphaRow; // alpha data for the row

The PRPackedBools should be moved to the end of the structure.

With those two changes, sr=tor.

Comment 5

[Aaron Kaluszka]

Attachment: patch

tor's comments addressed.
biesi, can you check this in?  Thanks.

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

tree's frozen currently... you have to ask for approval or wait until 1.9alpha

Comment 7

[Asa Dotzler [:asa]]

Aaron, can you tell us what value this adds to 1.8b2, what kind of risk it
poses, and what kind of testing's been done on the patch?

Comment 8

[Aaron Kaluszka]

Adds: XBM cursors can now be specified in CSS and their hotspots will work
properly instead of being set to the origin (see bug 38447 for discussion on CSS
cursors)
Risk: minimal (if this is broken then the original CUR cursor code is likely
broken since it was basically copied)
Testing: I've built and tested the patch with various valid and invalid files to
check for crashes or bad memory allocation and didn't find any problems.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 180021 [details] [diff] [review]
patch

a=asa

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp;
/cvsroot/mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.cpp,v  <-- 
nsXBMDecoder.cpp
new revision: 1.15; previous revision: 1.14
done
Checking in modules/libpr0n/decoders/xbm/nsXBMDecoder.h;
/cvsroot/mozilla/modules/libpr0n/decoders/xbm/nsXBMDecoder.h,v  <--  nsXBMDecoder.h
new revision: 1.6; previous revision: 1.5
done