Refactor/simplify the "delayedFallback" handling in the default viewer #12056
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Snuffleupagus
force-pushed
the
_delayedFallback
branch
from
c53d3d1
to
d5e0f6f
Compare
There's a few things that could be improved in the current implementation, such as: - It's currently necessary to *both* manually track the `featureId`s which should trigger delayed fallback, as well as manually report telemetry in affected cases. Obviously there's only two call-sites as of now (forms and javaScript), but it still feels somewhat error-prone especially if more cases were to be added in the future. To address this, this patch adds a new (private) method which abstracts away these details from the call-sites. - Generally, it also seems nice to reduce *and* simplify the amount of state we need to store on `PDFViewerApplication` in order to support the "delayedFallback" functionality. Also, having to *manually* work with the "delayedFallback"-array in multiple places feels cumbersome and makes e.g. the `PDFViewerApplication.fallback` method less clear as to its behaviour. - Having code *outside* of `PDFViewerApplication`, i.e. in the event handlers, directly access properties which are marked as "private" via a leading underscore doesn't seem that great in general. Furthermore, having the event handlers directly deal with that should be internal state also seem unfortunate. To address this, the patch will instead make use of a new `PDFViewerApplication.triggerDelayedFallback` callback. - There's at least one code-path in the viewer, see `PDFViewerApplication.error`, where `fallback` can be called without an argument. It's currently possible (although maybe somewhat unlikely) that such a call *could* be overridden by the `featureId` of a pending "delayedFallback" call, thus not reporting the *correct* fallback reason. - The "delayedFallback"-state weren't being reset on document close (which shouldn't affect Firefox, but nonetheless it ought to be fixed).
Snuffleupagus
force-pushed
the
_delayedFallback
branch
from
d5e0f6f
to
9e0000f
Compare
…target Given the dummy-methods on `DefaultExternalServices`, there's no longer any compelling reason not to (attempt to) report telemetry unconditionally. The only larger change consists of moving the `KNOWN_VERSIONS` and `KNOWN_GENERATORS` arrays ouf of the `PDFViewerApplication._initializeMetadata` method. *Please note:* Most of this patch consists of whitespace-only changes.
Snuffleupagus
force-pushed
the
_delayedFallback
branch
from
9e0000f
to
f9157ec
Compare
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/b647bc048bab574/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/b647bc048bab574/output.txt Total script time: 3.36 mins Published |
timvandermeij
approved these changes
Jul 8, 2020
|
Thank you for improving this code! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There's a few things that could be improved in the current implementation, such as:
It's currently necessary to both manually track the
featureIds which should trigger delayed fallback, as well as manually report telemetry in affected cases.Obviously there's only two call-sites as of now (forms and javaScript), but it still feels somewhat error-prone especially if more cases were to be added in the future. To address this, this patch adds a new (private) method which abstracts away these details from the call-sites.
Generally, it also seems nice to reduce and simplify the amount of state we need to store on
PDFViewerApplicationin order to support the "delayedFallback" functionality.Also, having to manually work with the "delayedFallback"-array in multiple places feels cumbersome and makes e.g. the
PDFViewerApplication.fallbackmethod less clear as to its behaviour.Having code outside of
PDFViewerApplication, i.e. in the event handlers, directly access properties which are marked as "private" via a leading underscore doesn't seem that great in general.Furthermore, having the event handlers directly deal with that should be internal state also seem unfortunate. To address this, the patch will instead make use of a new
PDFViewerApplication.triggerDelayedFallbackcallback.There's at least one code-path in the viewer, see
PDFViewerApplication.error, wherefallbackcan be called without an argument.It's currently possible (although maybe somewhat unlikely) that such a call could be overridden by the
featureIdof a pending "delayedFallback" call, thus not reporting the correct fallback reason.The "delayedFallback"-state weren't being reset on document close (which shouldn't affect Firefox, but nonetheless it ought to be fixed).
Smaller diff with https://github.com/mozilla/pdf.js/pull/12056/files?w=1