Avoid reading the "disablePreferences"/"locale" options, when initializing the viewer, in extension builds #11825
Conversation
…izing the viewer, in extension builds These two `AppOptions` are only defined in GENERIC builds, hence it's completely unnecessary to check them in the extension builds (e.g. MOZCENTRAL and CHROME). Also, simply let the "printResolution" option be defined in all builds since it's being accessed in `web/firefox_print_service.js` as well.
|
/botio-linux preview |
From: Bot.io (Linux m4)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.67.70.0:8877/3a46f5548637a2a/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/3a46f5548637a2a/output.txt Total script time: 2.56 mins Published |
| @@ -229,7 +229,11 @@ const PDFViewerApplication = { | |||
| * @private | |||
| */ | |||
| async _readPreferences() { | |||
| if (AppOptions.get("disablePreferences") === true) { | |||
| if ( | |||
| (typeof PDFJSDev === "undefined" || | |||
There was a problem hiding this comment.
I wonder what the background is for this change. Did you measure slowdowns here in the viewer? The app options are a simple/cheap lookup, so I'm having trouble figuring out if avoiding this check is beneficial given that the added code, in my opinion, doesn't improve readability... There might be a good reason for it, I'm just not seeing it right now, so perhaps you can shed some light on that?
There was a problem hiding this comment.
I wonder what the background is for this change.
Because to me it seems ridiculous to read something that we know will be undefined in e.g. MOZCENTRAL builds.
The app options are a simple/cheap lookup,
Yes, they should be cheap enough that that wasn't really the main motivation.
[...] if avoiding this check is beneficial given that the added code,
To me these added preprocessor checks actually helps reading/reasoning about the code, since it's no longer necessary to look at web/app_options.js to know that this particular option should only be defined in GENERIC builds.
[...] doesn't improve readability...
Well, similar pre-processor statements exists all over the code-base in both the web/ and src/ folders; I'm really not finding these particular cases any worse (or less readable) than lots of other code :-)
One example, from the viewer, is where the "locale" hash parameter is handle:
Lines 313 to 320 in a13db5d
There was a problem hiding this comment.
To me these added preprocessor checks actually helps reading/reasoning about the code, since it's no longer necessary to look at web/app_options.js to know that this particular option should only be defined in GENERIC builds.
Hm, that is a fair point indeed.
|
Thanks! |
These two
AppOptionsare only defined in GENERIC builds, hence it's completely unnecessary to check them in the extension builds (e.g. MOZCENTRAL and CHROME).Also, simply let the "printResolution" option be defined in all builds since it's being accessed in
web/firefox_print_service.jsas well.