517543 - need a STATUS_DECODE_COMPLETE for imgIRequest

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Bobby Holley (:bholley)]

Right now checking GetAnimated on imgIRequests doesn't tell us very much, because the image might be in the process of being decoded. STATUS_FRAME_COMPLETE doesn't help here, because that only says we've got the _first_ frame, but there might be more. Under decode-on-draw and discarding, STATUS_LOAD_COMPLETE doesn't help either, meaning that this consumer is wrong:
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp#2182

I'm not sure about this one:
http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHTMLImageAccessible.cpp#119

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

GetAnimated is on imgIContainer ;)
And the method should throw some error value if it is unknown whether
the image is animated. Probably NS_ERROR_NOT_AVAILABLE.

Comment 2

[Bobby Holley (:bholley)]

Attachment: patch v1

adding a patch. flagging joe for review.

Comment 3

[Bobby Holley (:bholley)]

Attachment: patch v2

slightly smarter patch to address smaug's concerns.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 401520 [details] [diff] [review]
patch v2

>-  PRUint32 statusBitsToClear = imgIRequest::STATUS_FRAME_COMPLETE;
>+  PRUint32 statusBitsToClear = imgIRequest::STATUS_FRAME_COMPLETE
>+                               | imgIRequest::STATUS_DECODE_COMPLETE;

Style nit, this should be 
  PRUint32 statusBitsToClear = imgIRequest::STATUS_FRAME_COMPLETE |
                               imgIRequest::STATUS_DECODE_COMPLETE;

Comment 5

[Joe Drew (not getting mail)]

Comment on attachment 401520 [details] [diff] [review]
patch v2

Don't know if I should have caught this before, but I'm not in love with OnStopDecode taking an nsresult as a parameter. Not sure why, but it feels wrong-ish. 

Something to keep in mind for future rearchitectings :)

Comment 6

[Bobby Holley (:bholley)]

(In reply to comment #5)
> (From update of attachment 401520 [details] [diff] [review])
> Don't know if I should have caught this before, but I'm not in love with
> OnStopDecode taking an nsresult as a parameter. Not sure why, but it feels
> wrong-ish. 
> 
> Something to keep in mind for future rearchitectings :)

It's very much wrong. I've quite a bit about it in bug 505385, bug 435296, and in various inline comments.

Comment 7

[Bobby Holley (:bholley)]

pushed to mc as 6ac9e403c8a5.