Originally the default preferences where simply placed in a JSON-file, checked into the repository, which over time became impractical, annoying, and error-prone to maintain; please see PR 10548.
While that improved the overall situation a fair bit, it however inherited one quite unfortunate property of the old JSON-based solution[1]: It's still not possible for *different* build targets to specify their *own* default preference values.
With some preferences, such as e.g. `enableScripting`, it's not inconceivable that you'd want to (at least) support build-specific default preference values. Currently that's not really possible, which is why this PR re-factors the default preferences generation to support this.
---
[1] This fact isn't really clear from the `AppOptions` implementation, unless you're familiar with the `gulpfile.js` code, which could lead to some confusion for those new to this part of the code-base.
A significant portion of the code-base has now been converted to use `let`/`const`, rather than `var`, hence it should be possible to simply enable the ESLint `no-var` rule globally.
This way we can ensure that new code won't accidentally use `var`, and it also removes the need to manually enable the rule in various folders.
Obviously it makes sense to continue the efforts to replace `var`, but that should probably happen on a file and/or folder basis.
Please note that this patch excludes the following code:
- The `extensions/` folder, since that seemed easiest for now (and I don't know exactly what the support situation is for the Chromium-extension).
- The entire `external/` folder is ignored, since most of it's currently excluded from linting.
For the code that isn't imported from elsewhere (and should be ignored), we should probably (at some point) bring the code up to the same linting/formatting standard as the rest of the code-base.
- Various files in the `test/` folder are ignored, as necessary, since the way that a lot of this code is loaded will require some care (or perhaps larger re-factoring) when removing `var` usage.
Please note that these changes were done automatically, using `gulp lint --fix`.
Given that the major version number was increased, there's a fair number of (primarily whitespace) changes; please see https://prettier.io/blog/2020/03/21/2.0.0.html
In order to reduce the size of these changes somewhat, this patch maintains the old "arrowParens" style for now (once mozilla-central updates Prettier we can simply choose the same formatting, assuming it will differ here).
Note that Prettier, purposely, has only limited [configuration options](https://prettier.io/docs/en/options.html). The configuration file is based on [the one in `mozilla central`](https://searchfox.org/mozilla-central/source/.prettierrc) with just a few additions (to avoid future breakage if the defaults ever changes).
Prettier is being used for a couple of reasons:
- To be consistent with `mozilla-central`, where Prettier is already in use across the tree.
- To ensure a *consistent* coding style everywhere, which is automatically enforced during linting (since Prettier is used as an ESLint plugin). This thus ends "all" formatting disussions once and for all, removing the need for review comments on most stylistic matters.
Many ESLint options are now redundant, and I've tried my best to remove all the now unnecessary options (but I may have missed some).
Note also that since Prettier considers the `printWidth` option as a guide, rather than a hard rule, this patch resorts to a small hack in the ESLint config to ensure that *comments* won't become too long.
*Please note:* This patch is generated automatically, by appending the `--fix` argument to the ESLint call used in the `gulp lint` task. It will thus require some additional clean-up, which will be done in a *separate* commit.
(On a more personal note, I'll readily admit that some of the changes Prettier makes are *extremely* ugly. However, in the name of consistency we'll probably have to live with that.)
With the removal of the (standalone) Firefox building code in PR 9566 (a year and a half ago), these files are now completely unused in the GitHub repository[1].
Hence it doesn't really seem necessary to keep fetching them with `gulp importl10n`, and the existing files in the `l10n` folder can also be removed (thanks to version control, they're easy enough to restore should the need ever arise).
The patch also allows an additional simplification, for the `gulp locale` and `gulp mozcentral` commands, since it's now possible to stop writing `l10n` files to the `extensions/firefox/` folder and instead just copy them similar to other build targets.
---
[1] They're obviously still used in `mozilla-central`, for fallback messages displayed through `PdfStreamConverter.jsm`, but that doesn't make it necessary to keep them *here* as far as I'm concerned.
With the removal of the (standalone) Firefox building code in PR 9566 (a year and a half ago), these files are now completely unused.
Hence it doesn't really make sense to keep building them as part of `gulp locale`, and the existing files in the `l10n` folder can also be removed (thanks to version control, they're easy enough to restore should the need ever arise).
Currently any editing of the preferences require updates in *three* separate files, which isn't a great developer experience to say the least.
This has annoyed me sufficiently to write this patch, which moves the definition of all preferences into `AppOptions` and adds a new `gulp` task to generate the `default_preferences.json` file for the builds where it's needed.
Based on the discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1521413, this patch simply removes the `ReadableStream` polyfill completely from MOZCENTRAL builds.
With this patch, the size of the `gulp mozcentral` build target is thus further reduced (building on PR 10470):
| | `build/mozcentral`
|-------|-------------------
|master | 3 339 666
|patch | 3 209 572
With https://bugzilla.mozilla.org/show_bug.cgi?id=1505122 landing in Firefox 65, the native `ReadableStream` implementation is now enabled by default in Firefox.
Obviously it would be nice to simply stop bundling the polyfill in MOZCENTRAL builds altogether, however given that it's still possible to disable[1] `ReadableStream` this is probably not a good idea just yet.
Nonetheless, now that native support is available, it seems unnecessary (and wasteful) to keep bundling the polyfill twice[2] in MOZCENTRAL builds. Hence this patch, which contains a suggest approach for packing the polyfill in a *separate* file which is then *only* loaded if/when needed.
With this patch, the size of the `gulp mozcentral` build target is thus reduced accordingly:
| | `build/mozcentral`
|-------|-------------------
|master | 3 461 089
|patch | 3 340 268
Besides the PDF.js files taking up less space in Firefox this way, the additional benefit is that there's (by default) less code that needs to be loaded and parsed when the PDF Viewer is used which also cannot hurt.
---
[1] In `about:config`, by toggling the `javascript.options.streams` preference.
[2] Once in the `build/pdf.js` file, and once in the `build/pdf.worker.js` file.
After the latest update of the minimum supported Firefox version, the development addon is no longer being signed. Hence this patch attempts to address that, by pinning the `minVersion` to a specific version number instead.
*Please note:* The version numbers were taken from https://addons.mozilla.org/en-US/firefox/pages/appversions/, so it ought to have worked.
At this point in time, trying to keep the development addon compatible with prior Firefox versions is already quite difficult and will become even harder very soon.
Please keep in mind that since Firefox 57, only WebExtensions are allowed/possible to install. The only exceptions are Firefox Nightly, with the `xpinstall.signatures.required` preference[1] set to `false`, and the (as of this writing) current ESR release.[2]
With the current compatibility situation, we thus need to effectively support both Nightly *and* ESR in the addon, while trying to keep up with current/upcoming changes in `mozilla-central`. With old addons no longer being officially supported, the amount of old code being removed/refactored is now increasing quite quickly.
*Please note:* The changes proposed here was to a large extent prompted by bugs such as:
- https://bugzilla.mozilla.org/show_bug.cgi?id=1431533 (already landed)
- https://bugzilla.mozilla.org/show_bug.cgi?id=767640 (already landed)
- https://bugzilla.mozilla.org/show_bug.cgi?id=1432992 (still open)
Upstreaming all of those changes for the `MOZCENTRAL` version of PDF.js, while also keeping the Firefox addon running in older versions of the browser, would quickly become non-trivial.
Since we're using the ruleset from `eslint-plugin-mozilla` for the addon files, which is enforced in `mozilla-central`, we must ensure that the neccessary changes are upstreamed to the PDF.js repo such that the `mozilla-central` version of PDF.js can still be updated without failures.
Trying to feature detect, for the `FIREFOX` build target, some of the changes in the referenced bugs would probably become both quite messy and (not to mention) difficult. E.g. with the upcoming and automatically defined `Cc, Ci, Cu, Cr` variables, any sort of feature detection might be tricky since those need to be defined in the global scope of the files in question.
Finally, given the amount of effort that we'd now need to spend to even attempt to keep the Firefox addon compatible, I just don't think it's worth the effort any more. Especially since the number of people that have, thus far, been doing this work is *very* low and those resources would be better spend elsewhere.
Unfortunately, this probably means that the development addon will no longer be compatible with release versions of Seamonkey. However the README has already mentioned, for quite some time, that support isn't guaranteed.
*In closing:* For all of the reasons mentioned above, I thus propose that we reduce the maintenance burden of the Firefox addon by only supporting the current Firefox Nightly.
---
[1] While the preference exists, and can be toggled, its value is ignored in non-Nightly versions of Firefox.
[2] There's unbranded builds of e.g. the release/beta versions of Firefox, where old and non-WebExtensions addons can be installed. However those builds can probably be assumed to be officially unsupported, and thus not recommended for users.
Since multiple empty lines is virtually unused in the code-base, and the few cases that do exist look like "typos", let's enforce greater consistency here; please see https://eslint.org/docs/rules/no-multiple-empty-lines.
While this doesn't actually fix the underlying issue, it should prevent the ESLint errors and thus make future PDF.js updates easier.
Compared to updating (and testing) the preprocessor, this seems like a reasonable workaround given its simplicity.
See Bug 1389443 for more information. This allows us to register
PdfJs without waiting for file IO in nsHandlerService to finish.
Once that file IO is finished, we can set everything up properly
and double-check the registration.
http://eslint.org/docs/rules/comma-danglehttp://eslint.org/docs/rules/object-curly-spacing
Given that we currently have quite inconsistent object formatting, fixing this in in one big patch probably wouldn't be feasible (since I cannot imagine anyone wanting to review that); hence I've opted to try and do this piecewise instead.
Please note: This patch was created automatically, using the ESLint `--fix` command line option. In a couple of places this caused lines to become too long, and I've fixed those manually; please refer to the interdiff below for the only hand-edits in this patch.
```diff
diff --git a/extensions/firefox/content/PdfStreamConverter.jsm b/extensions/firefox/content/PdfStreamConverter.jsm
index ea91a71a..0d59dad1 100644
--- a/extensions/firefox/content/PdfStreamConverter.jsm
+++ b/extensions/firefox/content/PdfStreamConverter.jsm
@@ -773,7 +773,8 @@ class RequestListener {
response = function sendResponse(aResponse) {
try {
var listener = doc.createEvent("CustomEvent");
- let detail = Cu.cloneInto({ response: aResponse, }, doc.defaultView);
+ let detail = Cu.cloneInto({ response: aResponse, },
+ doc.defaultView);
listener.initCustomEvent("pdf.js.response", true, false, detail);
return message.dispatchEvent(listener);
} catch (e) {
```
This is a downstream change introduced in [1]. That mozilla bug is adding a new property to channel's loadinfo object (nsILoadInfo) that protocol handlers has to set on channels when originalURI on the result channel is set to a different URI than the channel has been created for.
Existence of the new property on nsILoadInfo depends on landing [1].
[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1319111
Please note that I used the addon debugger to set a breakpoint in the `unload` function, in order to ensure that `this` still correctly refers to the `FindEventManager` scope.
In https://bugzilla.mozilla.org/show_bug.cgi?id=1355216, the *third* parameter of `Services.obs.addObserver` was made optional.
However, omitting it in Firefox versions *without* that patch causes failures that completely prevents the addon from working (it won't even load).
As far as I can tell, there isn't *any* way to detect ahead of time if the third parameter can be safely omitted, hence we're forced to fallback to manually checking the version number :-(
*Note:* Since the `PdfJs.jsm` file is only used in the `MOZCENTRAL` build, we at least don't need to add any compatibility hacks there.
