Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Avoid reading the "disablePreferences"/"locale" options, when initializing the viewer, in extension builds #11825

Merged
merged 1 commit into from
Apr 21, 2020
+18 −15
Merged

Avoid reading the "disablePreferences"/"locale" options, when initializing the viewer, in extension builds #11825

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions web/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,11 @@ const PDFViewerApplication = {
* @private
*/
async _readPreferences() {
if (AppOptions.get("disablePreferences") === true) {
if (
(typeof PDFJSDev === "undefined" ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

pdf.js/web/app.js

Lines 313 to 320 in a13db5d

// It is not possible to change locale for the (various) extension builds.
if (
(typeof PDFJSDev === "undefined" ||
PDFJSDev.test("!PRODUCTION || GENERIC")) &&
"locale" in hashParams
) {
AppOptions.set("locale", hashParams.locale);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PDFJSDev.test("!PRODUCTION || GENERIC")) &&
AppOptions.get("disablePreferences")
) {
// Give custom implementations of the default viewer a simpler way to
// opt-out of having the `Preferences` override existing `AppOptions`.
return;
Expand Down Expand Up @@ -328,9 +332,11 @@ const PDFViewerApplication = {
* @private
*/
async _initializeL10n() {
this.l10n = this.externalServices.createL10n({
locale: AppOptions.get("locale"),
});
this.l10n = this.externalServices.createL10n(
typeof PDFJSDev === "undefined" || PDFJSDev.test("!PRODUCTION || GENERIC")
? { locale: AppOptions.get("locale") }
: null
);
const dir = await this.l10n.getDirection();
document.getElementsByTagName("html")[0].dir = dir;
},
Expand Down
19 changes: 8 additions & 11 deletions web/app_options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@ const defaultOptions = {
value: false,
kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
},
/**
* The `disablePreferences` is, conditionally, defined below.
*/
enablePermissions: {
/** @type {boolean} */
value: false,
kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
},
/**
* The `disablePreferences` is, conditionally, defined below.
*/
enablePrintAutoRotate: {
/** @type {boolean} */
value: false,
Expand Down Expand Up @@ -110,9 +110,11 @@ const defaultOptions = {
value: false,
kind: OptionKind.VIEWER + OptionKind.PREFERENCE,
},
/**
* The `printResolution` is, conditionally, defined below.
*/
printResolution: {
/** @type {number} */
value: 150,
kind: OptionKind.VIEWER,
},
renderer: {
/** @type {string} */
value: "canvas",
Expand Down Expand Up @@ -252,11 +254,6 @@ if (
value: typeof navigator !== "undefined" ? navigator.language : "en-US",
kind: OptionKind.VIEWER,
};
defaultOptions.printResolution = {
/** @type {number} */
value: 150,
kind: OptionKind.VIEWER,
};
}

const userOptions = Object.create(null);
Expand Down