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

Handle errors in the "Loading by ref" code-path in PartialEvaluator.loadFont #15169

Merged
merged 1 commit into from
Jul 15, 2022

Conversation

Snuffleupagus
Copy link
Collaborator

Note how we currently throw a "raw" Error, which is problematical since all of the PartialEvaluator.loadFont call-sites expect a Promise to be returned. Furthermore, this also means that we don't benefit from the fallback code-path that now exists below.

Please note: Unfortunately I don't have a test-case that fails without this patch, since it's something I happened to notice when reading the code while working on another patch.

@Snuffleupagus
Copy link
Collaborator Author

/botio-windows test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/1a668379f9c712a/output.txt

src/core/evaluator.js Outdated Show resolved Hide resolved
@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/1a668379f9c712a/output.txt

Total script time: 29.98 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Integration Tests: FAILED
  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/1a668379f9c712a/reftest-analyzer.html#web=eq.log

Copy link
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM

…loadFont`

Note how we currently throw a "raw" Error, which is problematical since all of the `PartialEvaluator.loadFont` call-sites expect a Promise to be returned. Furthermore, this also means that we don't benefit from the fallback code-path that now exists below.

*Please note:* Unfortunately I don't have a test-case that fails without this patch, since it's something I happened to notice when reading the code while working on another patch.
@Snuffleupagus Snuffleupagus merged commit 75b8647 into mozilla:master Jul 15, 2022
@Snuffleupagus Snuffleupagus deleted the loadFont-fontRef branch July 15, 2022 14:41
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.

3 participants