[api-minor] Fixes behaviour of DOMCanvasFactory to return {canvas, context}. #8036
Conversation
| canvasEntry = this.canvasFactory.create(width, height); | ||
| this.cache[id] = canvasEntry; | ||
| } | ||
| if (trackTransform) { |
There was a problem hiding this comment.
Previously, this was inside the else branch. Shouldn't it be there now as well instead of outside of it?
There was a problem hiding this comment.
Please refer to http://logs.glob.uno/?c=pdfjs#c56734.
There was a problem hiding this comment.
For node-canvas, the canvas and context must be re-created. It will fine to that after the if.
There was a problem hiding this comment.
Good, let's keep it like this then. Thanks for clearing up the confusion!
src/display/dom_utils.js
Outdated
| return canvas; | ||
| return { | ||
| canvas: canvas, | ||
| context: context |
There was a problem hiding this comment.
Nit: add a trailing comma here, i.e., context: context,. This is to avoid unnecessarily line changes in the diff later on if new items are added to this object.
| canvas.height = 0; | ||
| canvasAndContextPair.canvas.width = 0; | ||
| canvasAndContextPair.canvas.height = 0; | ||
| canvasAndContextPair.canvas = null; |
There was a problem hiding this comment.
Do we need this? I think zeroing the width and height should be enough, just like in the previous version.
There was a problem hiding this comment.
loosing DOM object reference is always a plus
There was a problem hiding this comment.
Good point. Let's keep this then.
There was a problem hiding this comment.
loosing DOM object reference is always a plus
Then we probably ought to get rid of the context in a similar way, or am I missing something?
There was a problem hiding this comment.
yes, context must be nulled as well
mukulmishra18
force-pushed
the
node-canvas
branch
from
c318afa to
41d092d
Compare
|
/botio test |
From: Bot.io (Linux)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/f7967e323e6ced6/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @Snuffleupagus received. Current queue size: 0 Live output at: http://54.215.176.217:8877/b69e35d4250f76f/output.txt |
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/b69e35d4250f76f/output.txt Total script time: 22.11 mins
|
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/f7967e323e6ced6/output.txt Total script time: 25.27 mins
|
yurydelendik
changed the title
[api-minor] Fixes behaviour of DOMCanvasFactory to return {canvas, context}.
Fixes the behaviour of
DOMCanvasFactoryto return {canvas, context} pair instead to make it work for node's canvas.