Closed Bug 1081012 Opened 5 years ago Closed 5 years ago

Move DecodePool and its helpers out of RasterImage and into their own source file

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

Details

Attachments

(1 file, 2 obsolete files)

DecodePool and its helper classes like DecodeDoneWorker and FrameNeededWorker are currently embedded inside RasterImage. This is undesirable purely from the perspective of code hygiene, since it clutters an already gigantic class. It has also encouraged overly-tight coupling between RasterImage and these classes. 

Since we're currently moving in the direction of supporting multiple decoders at the same time for a single RasterImage, that coupling needs to be loosened, and I think a good preliminary step will be to move DecodePool and its helpers into their own source file.
Here's the patch. There's a try job here:

https://tbpl.mozilla.org/?tree=Try&rev=e470cdea7630
Attachment #8505181 - Flags: review?(tnikkel)
This patch needed a rebase.
Attachment #8505857 - Flags: review?(tnikkel)
Attachment #8505181 - Attachment is obsolete: true
Attachment #8505181 - Flags: review?(tnikkel)
Comment on attachment 8505857 [details] [diff] [review]
Move DecodePool and related helpers out of RasterImage

I read over this quickly based on your assertion that this is mostly just code movement. So if there is anything more complicated then simple code movement please let me know.
Attachment #8505857 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tn) from comment #3)
> Comment on attachment 8505857 [details] [diff] [review]
> Move DecodePool and related helpers out of RasterImage
> 
> I read over this quickly based on your assertion that this is mostly just
> code movement. So if there is anything more complicated then simple code
> movement please let me know.

Yeah, that's correct. I have just moved the code, changed formatting, and made trivial changes like switching to type-safe enums. Bigger changes that could actually impact behavior will happen in followups.

Thanks for the review!
OK, I rebased this patch and did some additional cleanup to make it conform to the style guide (things like adding braces around |if| statements). I also moved the helper runnables *above* the DecodePool code in DecodePool.cpp, which let me get rid of some forward declarations and other cruft.

This should now be ready to go if try looks good. Here's a test that this builds everywhere:

https://tbpl.mozilla.org/?tree=Try&rev=282329dd8bb4
Attachment #8505857 - Attachment is obsolete: true
Here's a test run on a subset of platforms:

https://tbpl.mozilla.org/?tree=Try&rev=ac1a9c0cc7db
https://hg.mozilla.org/mozilla-central/rev/27b056083099
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.