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.
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 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.
As described in the issue, having a DOM element with `id=page2` (or any other number) will automatically cause that element to become linkable through the URL hash. That's currently leading to some confusing and outright wrong behaviour, since it obviously only works for pages that have been loaded and rendered.
For PDF documents the only officially supported way to reference a particular page through the URL hash is using the `#page=2` format, which also works for all pages regardless if they're loaded or not.
As far as I can tell there's nothing in the PDF.js default viewer that actually depends on the page/thumbnail `id` at this point in time, hence why I believe that this removal ought to be safe.
Just as a pre-caution this patch adds an `aria-label` to the page canvas, similar to the thumbnail canvas/image, to at least keep this information in the DOM.
Currently *all* users of `pdfjs-dist` are forced to install the `webpack` and `worker-loader` packages, despite the fact that they are *only* relevant if the `webpack.js` file is being used (with a custom Webpack build).
This really doesn't seem great, especially since those packages are the only remaining dependencies in the `pdfjs-dist` library, and it thus seem more reasonable overall that Webpack users handle those dependencies themselves.
To prevent unnecessarily cryptic runtime failures, when people update to newer `pdfjs-dist` versions, the `webpack.js` file was updated to explicitly check for the existence of the `worker-loader` package and error otherwise.
Furthermore, note that `webpack` was only listed as a dependency because of the `worker-loader` package itself (see issue 9248).
Obviously these changes may not be seen as great by Webpack users who rely on `pdfjs-dist`, since it forces them to handle the dependencies themselves, however it should improve things considerably for "general" users of `pdfjs-dist` by not burdening them with unnecessary dependencies.
These sort of changes are also in line with other recent changes, see PR 11418, which removed built-in fake worker loader code for specific JS builders/bundlers/frameworks. This work was prompted not only by a desire to simplify/clean-up old code, but also to lessen future support burden since the PDF.js contributors cannot be assumed to be experts in various JS bundlers.
In order to eventually get rid of SystemJS and start using native `import`s instead, we'll need to provide "complete" file identifiers since otherwise there'll be MIME type errors when attempting to use `import`.
When working on PR 11463 I couldn't help thinking that the `Array.prototype.some` callback function, used when determining the generator, was somewhat difficult to read with its partly unused and strangely named parameters.
This is not only useful to have one format for consistency, but also to
be able to quickly search for colors for e.g., finding duplicates or
when tweaking the CSS for custom deployments.
