Remove Decoder::GetDecodeTotallyDone()

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

unspecified
mozilla43
Points:
---

Firefox Tracking Flags

(firefox43 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

3 years ago
We should rework the code in RasterImage::FinalizeDecoder so that Decoder::GetDecodeTotallyDone() is unnecessary, and then remove it. I would've made this change in bug 1187386, but since that's a refactoring bug I wanted to minimize the risk of changing behavior in that bug.
(Assignee)

Comment 1

3 years ago
Created attachment 8648317 [details] [diff] [review]
(Part 1) - Simplify the condition that determines whether we set RasterImage::mHasBeenDecoded

I think we can, and should, use a simpler condition here. Basically, if the
decoder was a full decoder, and it finished and wasn't aborted early, then we
can set mHasBeenDecoded to true. It doesn't matter whether we encountered an
error or not. I don't see any good reason to have different behavior if an error
was encountered, so that issue is ignored in the new condition.

This does mean that we're changing our behavior a little bit here, because
previously if a full decoder finished but did not set Decoder::mDecodeDone, we
wouldn't set RasterImage::mHasBeenDecoded. This seems pointless, though, because
this would always result from a deterministic error condition, so we effectively
*have* decoded the image to the extent that we can. And doing it the way we do
now hurts us, because it means that seriously corrupt, but small, images could
potentially get decoded on the main thread repeatedly when SYNC_DECODE_IF_FAST
is passed, since mHasBeenDecoded will never get set.
Attachment #8648317 - Flags: review?(tnikkel)
(Assignee)

Comment 2

3 years ago
Created attachment 8648318 [details] [diff] [review]
(Part 2) - Eliminate the nsresult return value from RasterImage::SetMetadata, since it's not used anymore

This is a pure refactoring patch: since we're not using the return value from
SetMetadata() anymore, let's get rid of it.
Attachment #8648318 - Flags: review?(tnikkel)
(Assignee)

Comment 3

3 years ago
Created attachment 8648320 [details] [diff] [review]
(Part 3) - For consistency, call DoError if SetMetadata sees a negative size

I noticed that we don't call DoError() if SetMetadata() sees a negative size.
This could trigger the assertion in FinalizeDecoder() that we always either
encounter an error or get the size in SetMetadata(). It's also inconsistent
behavior. So let's fix that.
Attachment #8648320 - Flags: review?(tnikkel)
Attachment #8648317 - Flags: review?(tnikkel) → review+
Attachment #8648318 - Flags: review?(tnikkel) → review+
Attachment #8648320 - Flags: review?(tnikkel) → review+
https://hg.mozilla.org/mozilla-central/rev/9bfb0efd0b6f
https://hg.mozilla.org/mozilla-central/rev/dc4bec055512
https://hg.mozilla.org/mozilla-central/rev/08dd09256e7e
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.