Only compile Type3 glyphs when Path2D is supported#15306
Merged
timvandermeij merged 1 commit intomozilla:masterfrom Aug 13, 2022
Merged
Only compile Type3 glyphs when Path2D is supported#15306timvandermeij merged 1 commit intomozilla:masterfrom
Path2D is supported#15306timvandermeij merged 1 commit intomozilla:masterfrom
Conversation
According to MDN `Path2D` is available in all browsers that we currently support, see https://developer.mozilla.org/en-US/docs/Web/API/Path2D#browser_compatibility Hence only Node.js is currently lagging behind here, and requires that we keep the old code as a fallback in the `compileType3Glyph` function. However, there's an open PR in the `node-canvas` repository for adding `Path2D` support. As far as I'm concerned, there's two possible solutions here: - We land this patch now, since it removes unnecessary code in e.g. the Firefox PDF Viewer, which means that compilation of Type3 glyphs will be disabled in Node.js until that PR is landed.[1] If users report bugs about Type3 glyphs looking "inconsistent" in Node.js and/or being slow to render, we could perhaps encourage them to upvote and otherwise help out getting that PR landed? - We wait for the mentioned PR to land *first*, before moving forward with this patch. Given that there's been no updates on that PR for almost two months, this alternative may possibly take a while. --- [1] Note that Type3 fonts are first of all not very common in PDF documents, and secondly that compilation only applies specifically to Type3 glyphs that contain /ImageMask-data (i.e. not all Type3 fonts are affected).
calixteman
approved these changes
Aug 12, 2022
Contributor
calixteman
left a comment
calixteman
left a comment
There was a problem hiding this comment.
From my Firefox point of view, this patch looks good to me.
Contributor
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.241.84.105:8877/ddd6529417b003e/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.193.163.58:8877/90b48f672c5de8a/output.txt |
From: Bot.io (Windows)FailedFull output at http://54.193.163.58:8877/90b48f672c5de8a/output.txt Total script time: 25.86 mins
|
From: Bot.io (Linux m4)FailedFull output at http://54.241.84.105:8877/ddd6529417b003e/output.txt Total script time: 26.79 mins
Image differences available at: http://54.241.84.105:8877/ddd6529417b003e/reftest-analyzer.html#web=eq.log |
timvandermeij
approved these changes
Aug 13, 2022
Contributor
|
Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
According to MDN
Path2Dis available in all browsers that we currently support, see https://developer.mozilla.org/en-US/docs/Web/API/Path2D#browser_compatibilityHence only Node.js is currently lagging behind here, and requires that we keep the old code as a fallback in the
compileType3Glyphfunction. However, there's an open PR in thenode-canvasrepository for addingPath2Dsupport; please see Automattic/node-canvas#2013As far as I'm concerned, there's two possible solutions here:
We land this patch now, since it removes unnecessary code in e.g. the Firefox PDF Viewer, which means that compilation of Type3 glyphs will be disabled in Node.js until that PR is landed.[1]
If users report bugs about Type3 glyphs looking "inconsistent" in Node.js and/or being slow to render, we could perhaps encourage them to upvote and otherwise help out getting that PR landed?
We wait for the mentioned PR to land first, before moving forward with this patch. Given that there's been no updates on that PR for almost two months, this alternative may possibly take a while.
[1] Note that Type3 fonts are first of all not very common in PDF documents, and secondly that compilation only applies specifically to Type3 glyphs that contain /ImageMask-data (i.e. not all Type3 fonts are affected).