Closed
Bug 1081012
Opened 10 years ago
Closed 10 years ago
Move DecodePool and its helpers out of RasterImage and into their own source file
Categories
(Core :: Graphics: ImageLib, defect)
Core
Graphics: ImageLib
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: seth, Assigned: seth)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Here's the patch. There's a try job here:
https://tbpl.mozilla.org/?tree=Try&rev=e470cdea7630
Attachment #8505181 -
Flags: review?(tnikkel)
Assignee | ||
Comment 2•10 years ago
|
||
This patch needed a rebase.
Attachment #8505857 -
Flags: review?(tnikkel)
Assignee | ||
Updated•10 years ago
|
Attachment #8505181 -
Attachment is obsolete: true
Attachment #8505181 -
Flags: review?(tnikkel)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
(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!
Assignee | ||
Comment 5•10 years ago
|
||
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
Assignee | ||
Updated•10 years ago
|
Attachment #8505857 -
Attachment is obsolete: true
Assignee | ||
Comment 6•10 years ago
|
||
Here's a test run on a subset of platforms:
https://tbpl.mozilla.org/?tree=Try&rev=ac1a9c0cc7db
Assignee | ||
Comment 7•10 years ago
|
||
Try looks good. Pushed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27b056083099
Comment 8•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•