Add an abstract base-class, which all the various Stream implementations inherit from #13303
Conversation
There was a problem hiding this comment.
Would it be an idea to maybe split the first five commits off into their own PR since they are only about converting existing files to standard classes, while the other commits are also creating and organizing new files? That would make this PR a bit more manageable.
Snuffleupagus
force-pushed
the
BaseStream
branch
5 times, most recently
from
054c0e1
to
8c78857
Compare
…ream.js` and into its own file
Snuffleupagus
force-pushed
the
BaseStream
branch
2 times, most recently
from
a258f60
to
56c3774
Compare
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b22e8bc1aff8c08/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/43dd513482b187e/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/b22e8bc1aff8c08/output.txt Total script time: 26.91 mins
Image differences available at: http://54.67.70.0:8877/b22e8bc1aff8c08/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/43dd513482b187e/output.txt Total script time: 29.26 mins
Image differences available at: http://3.101.106.178:8877/43dd513482b187e/reftest-analyzer.html#web=eq.log |
…ons inherit from By having an abstract base-class, it becomes a lot clearer exactly which methods/getters are expected to exist on all Stream instances. Furthermore, since a number of the methods are *identical* for all Stream implementations, this reduces unnecessary code duplication in the `Stream`, `DecodeStream`, and `ChunkedStream` classes. For e.g. `gulp mozcentral`, the *built* `pdf.worker.js` files decreases from `1 619 329` to `1 616 115` bytes with this patch-series.
…tations The way that `getBaseStreams` is currently handled has bothered me from time to time, especially how we're checking if the method exists before calling it. By adding a dummy `BaseStream.getBaseStreams` method, and having the call-sites simply check the return value, we can improve some of the relevant code. Note in particular how the `ObjectLoader._walk` method didn't actually check that the data in question is a Stream instance, and instead only checked the `currentNode` (which could be anything) for the existence of a `getBaseStreams` property.
…ream`/`JpxStream` constructors For all of the other `DecodeStream`s we're not passing in a `Dict`-instance manually, but instead get it from the `stream`-parameter. Hence there's no particularly good reason, as far as I can tell, to not do the same thing in `Jbig2Stream`/`JpegStream`/`JpxStream` as well.
Snuffleupagus
force-pushed
the
BaseStream
branch
from
56c3774
to
fb07755
Compare
Looking at the `ChunkedStream` implementation, it's basically a "regular" `Stream` but with added functionality in order to deal with fetching/loading of missing data. Hence, by letting `ChunkedStream` extend `Stream`, we can remove some duplicate methods from the `ChunkedStream` class.
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/867a598fc00c957/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://3.101.106.178:8877/8b5c7b432c0f809/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.67.70.0:8877/867a598fc00c957/output.txt Total script time: 25.79 mins
Image differences available at: http://54.67.70.0:8877/867a598fc00c957/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)FailedFull output at http://3.101.106.178:8877/8b5c7b432c0f809/output.txt Total script time: 29.69 mins
Image differences available at: http://3.101.106.178:8877/8b5c7b432c0f809/reftest-analyzer.html#web=eq.log |
|
Looks good so far. I'll try to do a full review this weekend since I'll need some time to carefully go over all commits :-) |
|
Thank you for all this work; looks much better! |
|
@timvandermeij Thanks a lot for reviewing this patch series; I realize that it cannot have been all that easy, or even fun for that matter! |
By having an abstract base-class, it becomes a lot clearer exactly which methods/getters are expected to exist on all Stream instances.
Furthermore, since a number of the methods are identical for all Stream implementations, this reduces unnecessary code duplication in the
Stream,DecodeStream, andChunkedStreamclasses.For e.g.
gulp mozcentral, the builtpdf.worker.jsfiles decreases from1 619 329to1 616 115bytes with this patch-series.The only way to review this is probably one commit at a time, with the
?w=1flag appended to the URL.