Closed Bug 1291071 Opened 4 years ago Closed 4 years ago

Stop passing a Decoder object to RasterImage::FinalizeDecoder() and rename it appropriately


(Core :: ImageLib, defect)

Not set



Tracking Status
firefox51 --- fixed


(Reporter: seth, Assigned: seth)




(6 files, 1 obsolete file)

RasterImage::FinalizeDecoder() is used to communicate the final results of the decoding process to RasterImage. However, the fact that it requires a real Decoder object requires some awkward workarounds when errors are detected outside of the Decoder itself. We should rework FinalizeDecoder() so it does not actually take a Decoder object as an argument, just the relevant data, the same way that NotifyProgress() works. Having done that, it'll probably also be appropriate to rename it.
All of these patches are pretty straightforward; we're just pulling all of the
data that FinalizeDecoder() needs to inspect off of the decoder and passing it
in directly.

This patch handles ImageMetadata.
Attachment #8777157 - Flags: review?(edwin)
Since we call NotifyProgress() one final time in FinalizeDecoder(), we need to
pass in the same stuff that we pass to NotifyProgress().
Attachment #8777158 - Flags: review?(edwin)
This patch makes us pass in telemetry data explicitly.
Attachment #8777159 - Flags: review?(edwin)
Decoder::SpeedHistogram() doesn't have the best design right now. It's
non-const, which prevented me from marking Decoder::Telemetry() const. It uses
an invalid telemetry ID value to indicate that we don't want to record telemetry
for a particular decoder. And at this point it should really be protected rather
than public. Let's fix all that stuff.
Attachment #8777160 - Flags: review?(edwin)
FinalizeDecoder() inspects what amounts to a few bits of state to determine
whether the decode succeeded and what kind of error handling to perform. This
patch bundles all that state up in a struct.
Attachment #8777163 - Flags: review?(edwin)
Now we've eliminated any reason to pass a decoder to FinalizeDecoder(), so we
can stop doing so. Now that it's more of a notification callback like
NotifyProgress(), it makes sense to rename it to match the same pattern, and to
make the function that sends the callback like NotifyProgress() does.
Attachment #8777166 - Flags: review?(edwin)
I noticed there's a line of dead code in NotifyProgress(); let's remove it.
Attachment #8777167 - Flags: review?(edwin)
Assignee: nobody → seth.bugzilla
It's worth noting that we'll be able to clean this stuff up more later. In this bug I'm just trying to preserve the same behavior / state / etc that we have now, but without giving RasterImage direct access to the decoder. Once the animated image refactoring stuff lands, there will be some further simplification opportunities.
Thanks for the reviews, Edwin!
Comment on attachment 8777167 [details] [diff] [review]
(Part 7) - Remove dead line of code in NotifyProgress().

I ended up folding this into bug 1291045.
Attachment #8777167 - Attachment is obsolete: true
Pushed by
(Part 1) - Move an assertion from RasterImage::FinalizeDecoder() to IDecodingTask::NotifyDecodeComplete(). r=edwin
(Part 1) - Pass ImageMetadata explicitly to FinalizeDecoder. r=edwin
(Part 2) - Pass decoder progress explicitly to FinalizeDecoder. r=edwin
(Part 3) - Pass telemetry explicitly to FinalizeDecoder. r=edwin
(Part 4) - Clean up Decoder::SpeedHistogram() and related code. r=edwin
(Part 5) - Pass the decoder's final status explicitly to FinalizeDecoder(). r=edwin
(Part 6) - Stop passing a decoder to FinalizeDecoder() and rename it NotifyDecodeComplete(). r=edwin
You need to log in before you can comment on or make changes to this bug.