need a STATUS_DECODE_COMPLETE for imgIRequest

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: bholley, Assigned: bholley)

Tracking

Trunk
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

9 years ago
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.

Updated

9 years ago
Blocks: 487667
Created attachment 401510 [details] [diff] [review]
patch v1

adding a patch. flagging joe for review.
Attachment #401510 - Flags: review?(joe)
Created attachment 401520 [details] [diff] [review]
patch v2

slightly smarter patch to address smaug's concerns.
Attachment #401510 - Attachment is obsolete: true
Attachment #401520 - Flags: review?(joe)
Attachment #401510 - Flags: review?(joe)

Comment 4

9 years ago
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 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 :)
Attachment #401520 - Flags: review?(joe) → review+
(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.
pushed to mc as 6ac9e403c8a5.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.