Convert the Preferences to an ES6 class
#8303
Conversation
|
/botio-linux preview |
| * Used for settings that should be applied to all opened documents, | ||
| * or every time the viewer is loaded. | ||
| */ | ||
| var Preferences = { | ||
| prefs: null, | ||
| isInitializedPromiseResolved: false, |
There was a problem hiding this comment.
This was simply removed, since there wasn't a single place in the code-base that actually used it for anything.
|
/botio-linux preview |
web/genericcom.js
Outdated
| @@ -23,10 +24,29 @@ if (typeof PDFJSDev !== 'undefined' && !PDFJSDev.test('GENERIC')) { | |||
|
|
|||
| var GenericCom = {}; | |||
|
|
|||
| class GenericPreferences extends BasePreferences { | |||
| _writeToStorage(prefObj) { | |||
| return new Promise((resolve) => { | |||
There was a problem hiding this comment.
This does not need to be an arrow function because this is not used.
There was a problem hiding this comment.
Hmm, I'm not sure how I managed to mess that up, but it's fixed now. Thanks for spotting it!
web/genericcom.js
Outdated
| } | ||
|
|
||
| _readFromStorage(prefObj) { | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
This does not need to be an arrow function because this is not used.
There was a problem hiding this comment.
Hmm, I'm not sure how I managed to mess that up, but it's fixed now. Thanks for spotting it!
web/chromecom.js
Outdated
| }); | ||
| }; | ||
|
|
||
| var getPreferences = (defaultPrefs) => { |
There was a problem hiding this comment.
Isn't it a problem that this is defined after it is used? I think it's better to move this above the if.
There was a problem hiding this comment.
Yes, it most certainly is, and it's also the reason that the Chrome extension wasn't working.
In hindsight, this is a really stupid mistake on my part, since I know that function expressions (in contrast with function declarations) aren't hoisted.
I did test this successfully in Chrome, but with a prior version of the patch that had the getPreferences code placed correctly. For reasons that I don't quite understand, I then figured that I'd try to reduced the size of the diff slightly, and moved it back without actually testing 😕
A really stupid mistake on my part, please see #8303 (comment) for an explanation of the problem. |
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/32a2a03f7a6df1e/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/32a2a03f7a6df1e/output.txt Total script time: 3.08 mins Published |
|
Thank you for working on this! Really nice solution with the abstract base class. No worries about the mistake; it happened to me as well and I'm glad the review process does its work to catch it. |
Convert the `Preferences` to an ES6 class
Please note that the new
BasePreferencesis an abstract class, and is explicitly not usable on its own, which is then sub-classed for the various build targets.Slightly smaller diff with https://github.com/mozilla/pdf.js/pull/8303/files?w=1.