Implement proper serialization of OptionalContentConfig#21349
Conversation
I happened to notice that the way the `OptionalContentConfig`-data handled in the PR that implements worker-rendering leaves a lot to be desired: - The way that the optional content state is handled is not correct, since that PR collects the "effective visibility" of the optional content groups rather than their *actual* internal state. - The necessary `OptionalContentConfig`-data is collected piecemeal in the API, which leads to quite frankly very messy code that's hard to read and will be even harder to maintain. The solution to all of these issues seem really simple though, just add a couple of `OptionalContentConfig` methods that serialize/de-serialize the necessary data. In the API calling `optionalContentConfig.serializable` will get *all* of the needed data for transferring to the worker-renderer, and once received there calling `OptionalContentConfig.fromSerializable(/* transferred data here */)` will create an `OptionalContentConfig` instance with the correct internal state. As part of this patch, to avoid increasing bundle-size unnecessarily, a couple of existing methods are stubbed out when the `OptionalContentConfig` class ends up in a worker-file (since they're unused there). This part assumes that the new worker-renderer is built correctly, note how the existing `pdf.worker.mjs` is handled in https://github.com/mozilla/pdf.js/blob/03eda70d7e75267bc58ee4f446a20f7f38792b47/gulpfile.mjs#L539-L545 (*Note:* Submitting a PR was a lot faster than trying to provide review comments, since writing this commit message took longer than writing the patch.)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21349 +/- ##
==========================================
- Coverage 81.42% 81.41% -0.01%
==========================================
Files 257 257
Lines 65351 65367 +16
==========================================
+ Hits 53212 53219 +7
- Misses 12139 12148 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/botio browsertest |
From: Bot.io (Linux m4)ReceivedCommand cmd_browsertest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.241.84.105:8877/a730ab861be62a6/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_browsertest from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.193.163.58:8877/f91ab7b6aa5a48f/output.txt |
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/a730ab861be62a6/output.txt Total script time: 19.40 mins
Image differences available at: http://54.241.84.105:8877/a730ab861be62a6/reftest-analyzer.html#web=eq.log |
From: Bot.io (Windows)SuccessFull output at http://54.193.163.58:8877/f91ab7b6aa5a48f/output.txt Total script time: 25.43 mins
|
timvandermeij
left a comment
There was a problem hiding this comment.
Looks good to me; thanks!
I happened to notice that the way the
OptionalContentConfig-data handled in the PR that implements worker-rendering leaves a lot to be desired:The way that the optional content state is handled is not correct, since that PR collects the "effective visibility" of the optional content groups rather than their actual internal state.
The necessary
OptionalContentConfig-data is collected piecemeal in the API, which leads to quite frankly very messy code that's hard to read and will be even harder to maintain.The solution to all of these issues seem really simple though, just add a couple of
OptionalContentConfigmethods that serialize/de-serialize the necessary data.In the API calling
optionalContentConfig.serializablewill get all of the needed data for transferring to the worker-renderer, and once received there callingOptionalContentConfig.fromSerializable(/* transferred data here */)will create anOptionalContentConfiginstance with the correct internal state.As part of this patch, to avoid increasing bundle-size unnecessarily, a couple of existing methods are stubbed out when the
OptionalContentConfigclass ends up in a worker-file (since they're unused there).This part assumes that the new worker-renderer is built correctly, note how the existing
pdf.worker.mjsis handled inpdf.js/gulpfile.mjs
Lines 539 to 545 in 03eda70
(Note: Submitting a PR was a lot faster than trying to provide review comments, since writing this commit message took longer than writing the patch.)