There's no particular reason for using the PDF.js helper function `createObjectURL`[1] in the debugger, instead of the native `URL.createObjectURL` directly, for a couple of reasons:
- The relevant code-path only applies to fonts loaded with the Font Loading API, and this isn't supported in IE anyway.
- The debugger can, since quite some time, not even be loaded in IE any more.
- General support for IE is now limited, and there's no guaratee that everything actually works.
---
[1] It provides a fallback for browsers with broken `Blob` support, which as usual means Internet Explorer :-P
In the current `AnnotationLayer` implementation, Popup annotations require that the parent annotation have already been rendered (otherwise they're simply ignored).
Usually the annotations are ordered, in the `/Annots` array, in such a way that this isn't a problem, however there's obviously no guarantee that all PDF generators actually do so. Hence we simply ensure, when rendering the `AnnotationLayer`, that the Popup annotations are handled last.
Interestingly the viewer already seem to work correctly as-is, with workers disabled and a non-standard `verbosity` level.
Hence this is possibly Node.js specific, but given that the issue is lacking *both* the PDF file in question and a runnable test-case, so this patch is essentially a best-effort guess at what the problem could be.
- Re-factor the "incorrect encoding" check, since this can be easily achieved using the general `findNextFileMarker` helper function (with a suitable `startPos` argument).
- Tweak a condition, to make it easier to see that the end of the data has been reached.
- Add a reference test for issue 1877, since it's what prompted the "incorrect encoding" check.
The other image decoders, i.e. the JBIG2 and JPEG 2000 ones, are using the common helper function `readUint16`. Most likely, the only reason that the JPEG decoder is doing it this way is because it originated *outside* of the PDF.js library.
Hence we can simply re-factor `src/core/jpg.js` to use the common `readUint16` helper function, which is especially nice given that the functionality was essentially *duplicated* in the code.
This covers cases that the `--fix` command couldn't deal with, and in a few cases (notably `src/core/jbig2.js`) the code was changed to use block-scoped variables instead.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/prefer-const
With the recent introduction of Prettier this sort of mass enabling of ESLint rules becomes a lot easier, since the code will be automatically reformatted as necessary to account for e.g. changed line lengths.
Note that this patch is generated automatically, by using the ESLint `--fix` argument, and will thus require some additional clean-up (which is done separately).
The debugging hash parameters[1] are intended to facilitate access to various tools/settings in PRODUCTION builds, protected by the `pdfBugEnabled` preference.
At this point, the remaining debugging hash parameters are mainly intended to allow access to the `PDFBug` tools and/or to quickly toggle certain larger features.
The "useOnlyCssZoom" functionality doesn't really seem to fit in with the rest of these hash parameters, since:
- This is, comparatively speaking, a minor viewer-specific feature.
- The zooming implementation will (almost) always fallback to CSS-only zooming, for any document, once the canvases becomes large enough. Hence, the majority of the CSS zooming feature can still be tested *directly* in any build of the viewer.
- After the initial implementation, years ago, the CSS-only zooming code in question hasn't changed much (or even at all), i.e. it doesn't seem like an active development target.[2]
- If the "useOnlyCssZoom" functionality was added today, it's unlikely that a hash parameter would've been added.
- Last, but not least, there's also a `useOnlyCssZoom` preference hence toggling this functionality shouldn't be too difficult (e.g. if someone needs to hack on it).
All in all, I'm thus suggesting that we remove the "useOnlyCssZoom" hash parameter.
---
[1] Originally these hash parameters could be used directly in any build, which was bad since it would allow any link to potentially disable functionality and/or reduce performance.
[2] If it had seen active development over the years, I'd be *much* more inclined to keep the hash parameter.
For people running e.g. Firefox with the `pdfBugEnabled` preference set, to allow quick access to debugging tools, this method will obviously run for every opened PDF file. However, in most cases the URL hash is empty and we can thus skip most of the parsing and simply return early instead.
These two cases should have been whitelisted prior to re-formatting respectively had the comments fixed afterwards, however I unfortunately missed them because of the massive size of the diff.
This removes a couple of, thanks to preceeding code, unnecessary `typeof PDFJSDev` checks, and also fixes a couple of incorrectly implemented (my fault) checks intended for `TESTING` builds.
After PR 9566, which removed all of the old Firefox extension code, the `FIREFOX` build flag is no longer used for anything.
It thus seems to me that it should be removed, for a couple of reasons:
- It's simply dead code now, which only serves to add confusion when looking at the `PDFJSDev` calls.
- It used to be that `MOZCENTRAL` and `FIREFOX` was *almost* always used together. However, ever since PR 9566 there's obviously been no effort put into keeping the `FIREFOX` build flags up to date.
- In the event that a new, Webextension based, Firefox addon is created in the future you'd still need to audit all `MOZCENTRAL` (and possibly `CHROME`) build flags to see what'd make sense for the addon.
While only the `MOZCENTRAL` builds will actually do anything meaningful with the telemetry data, none of the code in question actually runs *at all* in e.g. development mode.[1]
This seems bad since it essentially means that this code is completely untested, despite being quite important for the built-in Firefox PDF viewer, and this thus ought to be fixed.
In this case, the explanation for the current state of the code should be "for historical reasons". Before the viewer was split into the current components and before the pre-processor was improved, back when all code resided in the `web/viewer.js` file, the telemetry reporting was done with *direct* `FirefoxCom` calls. However, with the dummy `DefaultExternalServices.reportTelemetry` method there's nothing actually preventing attempting to report telemetry in any type of build.
NOTE: By running this code in GENERIC builds as well, in addition to just locally, the *viewer* part of telemetry reporting becomes tested e.g. in preview builds too which should help with reviewing.
---
[1] When fixing bug 1606566, I had to edit the relevant `PDFJSDev` checks to be able to actually test the changes locally.
With https://bugzilla.mozilla.org/show_bug.cgi?id=844349 now being fixed in Firefox, the textLayer will now actually stay hidden as intended regardless of the browser settings.
Hence it should no longer be necessary to display the fallback bar, nor print a warning in the console, for documents which contains a textLayer.
Besides removing the `supportsDocumentColors` methods in the default viewer, we can also remove a now unused l10n string.
This particular JSDoc comment is fairly old and it also contains some now unrelated/confusing information.
The only way to *guarantee* that the PDF.js library works as expected is to correctly set the global `workerSrc`[1], hence giving the impression that the option isn't strictly necessary is thus incorrect.
---
[1] Since advertising the fallbackWorkerSrc functionality definitely seems like the *wrong* thing to do.
This way we'll benefit from the existing font caching, and can thus avoid re-creating a fallback font over and over again during parsing.
(Thece changes necessitated the previous patch, since otherwise breakage could occur e.g. with fake workers.)
This is beneficial in situations where the Worker is being re-used, for example with fake workers, since it ensures that things like font resources are actually released.
This rule is already enabled in mozilla-central, and helps avoid some confusing formatting, see https://searchfox.org/mozilla-central/rev/9e45d74b956be046e5021a746b0c8912f1c27318/tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js#209-210
With the recent introduction of Prettier some of the existing nested ternary statements became even more difficult to read, since any possibly helpful indentation was removed.
This particular ESLint rule wasn't entirely straightforward to enable, and I do recognize that there's a certain amount of subjectivity in the changes being made. Generally, the changes in this patch fall into three categories:
- Cases where a value is only clamped to a certain range (the easiest ones to update).
- Cases where the values involved are "simple", such as Numbers and Strings, which are re-factored to initialize the variable with the *default* value and only update it when necessary by using `if`/`else if` statements.
- Cases with more complex and/or larger values, such as TypedArrays, which are re-factored to let the variable be (implicitly) undefined and where all values are then set through `if`/`else if`/`else` statements.
Please find additional details about the ESLint rule at https://eslint.org/docs/rules/no-nested-ternary
Note that this will still allow the FBF tests to run locally, and also on the bots when invoked with `test`/`browsertest` (to not lose all the FBF test-coverage), but will no longer prevent `makeref` from running successfully on the bots.
