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

Re-factor the idFactory functionality, used in the core/-code, and move the fontID generation into it #12070

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

Snuffleupagus
Copy link
Collaborator

Note how the getFontID-method in src/core/fonts.js is completely global, rather than properly tied to the current document. This means that if you repeatedly open and parse/render, and then close, even the same PDF document the fontIDs will still be incremented continuously.

For comparison the createObjId method, on idFactory, will always create a consistent id, assuming of course that the document and its pages are parsed/rendered in the same order.

In order to address this inconsistency, it thus seems reasonable to add a new createFontId method on the idFactory and use that when obtaining fontIDs. (When the current getFontID method was added the idFactory didn't actually exist yet, which explains why the code looks the way it does.)
Please note: Since the document id is (still) part of the loadedName, it's thus not possible for different documents to have identical font names.

Implements the idea from #11610 (comment)

…d move the `fontID` generation into it

Note how the `getFontID`-method in `src/core/fonts.js` is *completely* global, rather than properly tied to the current document. This means that if you repeatedly open and parse/render, and then close, even the *same* PDF document the `fontID`s will still be incremented continuously.

For comparison the `createObjId` method, on `idFactory`, will always create a *consistent* id, assuming of course that the document and its pages are parsed/rendered in the same order.

In order to address this inconsistency, it thus seems reasonable to add a new `createFontId` method on the `idFactory` and use that when obtaining `fontID`s. (When the current `getFontID` method was added the `idFactory` didn't actually exist yet, which explains why the code looks the way it does.)
*Please note:* Since the document id is (still) part of the `loadedName`, it's thus not possible for different documents to have identical font names.
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jul 7, 2020

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/bce65feb7888441/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 7, 2020

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/ca34ffe211eb1ee/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 7, 2020

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/bce65feb7888441/output.txt

Total script time: 26.27 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/bce65feb7888441/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

pdfjsbot commented Jul 7, 2020

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/ca34ffe211eb1ee/output.txt

Total script time: 29.69 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/ca34ffe211eb1ee/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@pdfjsbot
Copy link

pdfjsbot commented Jul 9, 2020

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/a18851b814d4ed0/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 9, 2020

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/a18851b814d4ed0/output.txt

Total script time: 3.37 mins

Published

@timvandermeij timvandermeij merged commit 38cf49b into mozilla:master Jul 9, 2020
@timvandermeij
Copy link
Contributor

Thank you for improving this!

@Snuffleupagus Snuffleupagus deleted the createFontId branch July 9, 2020 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants