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 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).
Sometimes we also used `@return`, but `@returns` is what the JSDoc
documentation recommends. Even though `@return` works as an alias, it's
good to use the recommended syntax and to be consistent within the
project.
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.
This patch ignores the recently added `disableOpenActionDestination` preference, since the latest PDF.js version found on the "Chrome Web Store" doesn't include it.
The intention with preferences such as `sidebarViewOnLoad`/`scrollModeOnLoad`/`spreadModeOnLoad` were always that they should be able to *unconditionally* override their view history counterparts.
Due to the way that these preferences were initially implemented[1], trying to e.g. force the sidebar to remain hidden on load cannot be guaranteed[2]. The reason for this is the use of "enumeration values" containing zero, which in hindsight was an unfortunate choice on my part.
At this point it's also not as simple as just re-numbering the affected structures, since that would wreak havoc on existing (modified) preferences. The only reasonable solution that I was able to come up with was to change the *default* values of the preferences themselves, but not their actual values or the meaning thereof.
As part of the refactoring, the `disablePageMode` preference was combined with the *adjusted* `sidebarViewOnLoad` one, to hopefully reduce confusion by not tracking related state separately.
Additionally, the `showPreviousViewOnLoad` and `disableOpenActionDestination` preferences were combined into a *new* `viewOnLoad` enumeration preference, to further avoid tracking related state separately.
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.
This is *really* the best that we can do here, since other proposed solutions would interfere with (and break) the painstakingly implemented browsing history that's present in the default viewer.
I'm still not convinced that this is a good idea in general, but this patch implements it in a way where it is possible to toggle[1] for users that wish to have this feature. In particular, there's a couple of reasons why I'm not finding this feature necessary/great:
- It's already possible to easily obtain the current hash, by simply clicking on the `viewBookmark` button at any time.
- Hash changes requires a bit of special handling[2], i.e. extra code, to prevent issues when the browser history is traversed (see `PDFHistory._popState`). Currently this is only necessary when the user has manually changed the hash, with this patch it will always be the case (assuming the feature is active).
- It's not always possible to change the URL when updating the browser history. For example: In the Firefox built-in viewer, the URL cannot be modified for local files (i.e. those using the `file://` protocol).
This leads to inconsistent behaviour, and may in some cases even result in errors being thrown and the history thus not updating, if the browser prevents changes to the URL during `pushState`/`replaceState` calls.
---
[1] Using the `historyUpdateUrl` viewer preference.
[2] This depends, to a great extent, on browsers always firing `popstate` events *before* `hashchange` events, which may or may not actually be guaranteed.
Given that it's really not clear to me if this is actually desired functionality in the default viewer, and considering that it doesn't fit in *great* with the way that `PDFHistory` is initialized, this feature is currently off by default[1].
---
[1] It's controlled with the `disableOpenActionDestination` Preference/AppOption.
This patch is the first step to be able to eventually get rid of the `attachDOMEventsToEventBus` function, by allowing `EventBus` instances to simply re-dispatch most[1] events to the DOM.
Note that the re-dispatching is purposely implemented to occur *after* all registered `EventBus` listeners have been serviced, to prevent the ordering issues that necessitated the duplicated page/scale-change events.
The DOM events are currently necessary for the `mozilla-central` tests, see https://hg.mozilla.org/mozilla-central/file/tip/browser/extensions/pdfjs/test, and perhaps also for custom deployments of the PDF.js default viewer.
Once this have landed, and been successfully uplifted to `mozilla-central`, I intent to submit a patch to update the test-code to utilize the new preference. This will thus, eventually, make it possible to remove the `attachDOMEventsToEventBus` functionality.
*Please note:* I've successfully ran all `mozilla-central` tests locally, with these patches applied.
---
[1] The exception being events that originated on the `window` or `document`, since those are already globally available anyway.
Without providing useful (custom) error messages for the `no-restricted-globals` rule, see https://eslint.org/docs/rules/no-restricted-globals, it's quite likely that the rule will be incorrectly disabled rather than the required globals being imported as intended.
To reduced duplication of the `no-restricted-globals` rule in multiple `.eslintrc` files, it's instead moved to the top-level `.eslintrc` file and disabled as needed on a folder/file basis outside of `/src` and `/web`.
This commit adds `scrollModeOnLoad` and `spreadModeOnLoad` preferences
that control the default viewer state when opening a new document for
the first time.
This commit also contains a minor refactoring of some of the option UI
rendering code in extensions/chromium/options/options.js, as I couldn't
bear creating two more functions nearly identical to the four that
already existed.
In a1cfa5f4d7, the textLayerMode
preference was introduced, to replace the disableTextLayer and
enhanceTextSelection preferences.
As a result, the text selection preference was no longer visible
in Chrome (because preferences are only rendered by default for
boolean preferences, not for enumerations).
This commit adds the necessary bits to
extensions/chromium/options/options.{html,js}
so that the textLayerMode preference can be changed again.
Also, migration logic has been added to move over preferences
from the old to the new names:
- In web/chromecom.js, the logic is added to translate
preferences that were set by an administrator (it is read-only,
so this layer is unavoidable).
- In extensions/chromium/options/migration.js, similar logic is
added, except in this case the preference storage is writable,
so this migration logic happens only once.
The "enhanced text selection" mode is still experimental, so it
has been marked as experimental to signal that there may be bugs.
The list of tasks that block promotion to stable is at #7584.
This partially reverts df0836b9b8.
The entry in preferences_schema.json is restored because that is
required to make managed preferences visible to the extension code.
The default key is still removed from default_preferences.json,
because this change only concerns the Chrome extension, not the
other parts of PDF.js. To account for the missing key, the
deprecated key was added back in chromecom.js
The key needs to be restored in preferences_schema.json too,
because that's the only way to make managed preferences visible.
I'm using `Object.assign`, which was introduced in Chrome 45,
so the preference module will break in Chrome 45 and earlier.
This is fine, because we do not support Chrome before 49.
Rather than having two different (but connected) options for the textLayer, I think that it makes sense to try and unify this. For example: currently if `disableTextLayer === true`, then the value of `enhanceTextSelection` is simply ignored.
Since PDF.js version `2.0` already won't be backwards compatible in lots of ways, I don't think that we need to worry about migrating existing preferences here.
This rule is available from https://www.npmjs.com/package/eslint-plugin-mozilla, and is enforced in mozilla-central. Note that we have the necessary `Array`/`String` polyfills and that most cases have already been fixed, see PRs 9032 and 9434.
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.
Merge ftp and file handler now their implementations are identical.
Remove redundant comment (the referenced Chrome bug has been fixed
a long time ago - https://crbug.com/302548 ).
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.
