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.

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

Add the possibility to change some pdfjs preferences from the viewer (bug 1908483) #18449

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

calixteman
Copy link
Contributor

No description provided.

@Snuffleupagus
Copy link
Collaborator

Would it not be more appropriate to restore (some of) the code removed in 1f9d1f3 instead?

@calixteman
Copy link
Contributor Author

calixteman commented Jul 17, 2024

Would it not be more appropriate to restore (some of) the code removed in 1f9d1f3 instead?

I didn't know the removed code existed and I forgot I reviewed this patch...

With an allow-list, I've the feeling that my code is slightly better because the change in m-c is limited to pdf.js.
That said, I could add an allow-list too, to make sure we avoid a change for an unexpected pref.

EDIT: I'm mostly talking about my patch in m-c... I can updated this PR for using preferences stuff.

@calixteman
Copy link
Contributor Author

@Snuffleupagus, I don't understand why we're writing all the prefs:

pdf.js/web/preferences.js

Lines 117 to 118 in 15b71b8

const prefs = AppOptions.getAll(OptionKind.PREFERENCE);
await this._writeToStorage(prefs);

I'd have expected to only write the changed ones, wdyt ?

@Snuffleupagus
Copy link
Collaborator

EDIT: I'm mostly talking about my patch in m-c... I can updated this PR for using preferences stuff.

Yes, for the viewer-side of things I believe it'd make sense to mostly revert that commit; however I think we can probably keep the reset-method disabled in MOZCENTRAL builds.

I'd have expected to only write the changed ones, wdyt ?

In the GENERIC viewer the prefs are stored as a string in localStorage, hence why all are written. (Perhaps we could special-case things for MOZCENTRAL builds, if that's helpful...)

web/preferences.js Outdated Show resolved Hide resolved
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

@calixteman calixteman merged commit e1f64a5 into mozilla:master Jul 18, 2024
7 checks passed
@calixteman calixteman deleted the bug1908483 branch July 18, 2024 08:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants