1187401 - Remove Decoder::GetDecodeTotallyDone()

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Seth Fowler [:seth] [:s2h]]

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.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: (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.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: (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.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: (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.

Comment 4

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c5872901ab5

Comment 5

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bfb0efd0b6f
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc4bec055512
https://hg.mozilla.org/integration/mozilla-inbound/rev/08dd09256e7e

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9bfb0efd0b6f
https://hg.mozilla.org/mozilla-central/rev/dc4bec055512
https://hg.mozilla.org/mozilla-central/rev/08dd09256e7e