[api-minor] Fix #7798: Refactor scratch canvas usage. #8002

mukulmishra18 · 2017-01-27T20:04:07Z

Fixes #7798, Refactoring scratch canvas usage to use both DOM canvas and npm canvas.

yurydelendik · 2017-01-27T20:11:20Z

src/display/dom_utils.js

@@ -33,6 +33,44 @@ var createValidAbsoluteUrl = sharedUtil.createValidAbsoluteUrl;

 var DEFAULT_LINK_REL = 'noopener noreferrer nofollow';

+if (typeof require === 'function') {
+  var Canvas = require('canvas');
+}


remove this require() call -- Canvas is not used anywhere

Actually it is used in the else block of create method, line 48.

yurydelendik · 2017-01-27T20:11:24Z

src/display/webgl.js

@@ -83,7 +84,8 @@ var WebGLUtils = (function WebGLUtilsClosure() {
    if (currentGL) {
      return;
    }
-    currentCanvas = document.createElement('canvas');


revert the change here -- it is used in the webgl context, you can add a comment that CanvasRactory is not needed here

yurydelendik · 2017-01-27T20:11:30Z

package.json

@@ -2,6 +2,7 @@
  "name": "pdf.js",
  "version": "0.8.0",
  "devDependencies": {
+    "canvas": "^1.6.2",


remove this package - we are not ready to use node.js canvas just yet

yurydelendik · 2017-01-27T20:12:43Z

src/display/text_layer.js

@@ -186,7 +187,8 @@ var renderTextLayer = (function renderTextLayerClosure() {
      return;
    }

-    var canvas = document.createElement('canvas');


revert the change here -- it is used to measure text length in the DOM, you can add a comment that CanvasRactory is not needed here

yurydelendik · 2017-01-27T20:13:20Z

src/display/font_loader.js

@@ -216,7 +218,8 @@ if (typeof PDFJSDev === 'undefined' || !PDFJSDev.test('MOZCENTRAL')) {

      var i, ii;

-      var canvas = document.createElement('canvas');


revert the change here -- it is used for hack to determine if font is loaded, you can add a comment that CanvasRactory is not needed here

yurydelendik · 2017-01-27T20:14:04Z

src/display/canvas.js

@@ -1454,7 +1449,8 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    get isFontSubpixelAAEnabled() {
      // Checks if anti-aliasing is enabled when scaled text is painted.
      // On Windows GDI scaled fonts looks bad.
-      var ctx = document.createElement('canvas').getContext('2d');
+      var canvasFactory = new CanvasFactory();


CanvasFactory must be created once per render() call and stored at CanvasGraphics.

Snuffleupagus

I've added a couple of review comments based on cursory looking at/testing the code.
Having briefly tested this locally, using the default tracemonkey.pdf file with gulp server, it seems that the current version of this patch unfortunately doesn't work as is.

Again, please remember to run tests (and use the viewer locally), since that helps catch errors as described below; see e.g. https://github.com/mozilla/pdf.js/wiki/Contributing#4-run-lint-and-testing.

Snuffleupagus · 2017-01-28T08:31:57Z

src/display/canvas.js

@@ -50,6 +50,7 @@ var TilingPattern = displayPatternHelper.TilingPattern;
 var getShadingPatternFromIR = displayPatternHelper.getShadingPatternFromIR;
 var WebGLUtils = displayWebGL.WebGLUtils;
 var hasCanvasTypedArrays = displayDOMUtils.hasCanvasTypedArrays;
+var DOMcanvasFactory = displayDOMUtils.DOMCanvasFactory;


Please change the capitalization to var DOMCanvasFactory = ... to be consistent with the actual factory name.

Okay, thanks.

Snuffleupagus · 2017-01-28T08:33:33Z

src/display/font_loader.js

  }
-}(this, function (exports, sharedUtil) {
+}(this, function (exports, sharedUtil, displayDOMUtils) {


Since you're not actually using displayDOMUtils anywhere in this file, please don't import it.

Snuffleupagus · 2017-01-28T08:37:02Z

src/display/api.js

 var Metadata = displayMetadata.Metadata;
 var getDefaultSetting = displayDOMUtils.getDefaultSetting;
-


Nit: Please don't delete this new line, since the imports should be separated from the definition of the constant just below.

Snuffleupagus · 2017-01-28T08:39:50Z

src/display/dom_utils.js

+    return this.canvas;
+  },
+
+  destroy: function DOMCanvasFactory_destroy() {


Please also remember to set the width/height of the canvas to 0 before removing it, since that will cause (at least) Firefox to free graphics resources quicker; refer to e.g.

pdf.js/src/display/canvas.js

Line 233 in 2b84fb7

// Zeroing the width and height causes Firefox to release graphics

.

Please also remember to set the width/height of the canvas to 0 before removing it, since that will cause (at least) Firefox to free graphics resources quicker

That is a good point, i didn't think about that. Thanks for pointing.

Snuffleupagus · 2017-01-28T08:42:31Z

src/display/webgl.js

@@ -83,6 +83,8 @@ var WebGLUtils = (function WebGLUtilsClosure() {
    if (currentGL) {
      return;
    }
+
+    // CanvasFactory is not needed here, as it is used in the webgl context.


Nit: let's capitalize this corrently, i.e. please use WebGL instead.

Snuffleupagus · 2017-01-28T09:08:09Z

src/display/canvas.js

@@ -478,6 +472,7 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    this.smaskCounter = 0;
    this.tempSMask = null;
    this.cachedCanvases = new CachedCanvases();
+    this.DOMcanvasFactory = new DOMcanvasFactory();


It may be better to call it this.canvasFactory, since I believe that one of the points of issue #7798 is to allow an API consumer to provide their own canvas factory (e.g. using Node canvas), and then a using the this.DOMCanvasFactory name here would be confusing.

Furthermore, I think that you also need to initialize the DOMCanvasFactory before CachedCanvases, since you'll need to do this.cachedCanvases = new CachedCanvases(this.canvasFactory);.
Please refer to https://github.com/mozilla/pdf.js/pull/8002/files#r98327854.

It may be better to call it this.canvasFactory, since I believe that one of the points of issue #7798 is to allow an API consumer to provide their own canvas factory (e.g. using Node canvas), and then a using the this.DOMCanvasFactory name here would be confusing.

Should i have to change this.DOMcanvasFactory with this.canvasFactory in the whole canvas.js file or just the specified place?

I'm not sure that I really understand what you're asking here, but you should (obviously) be consistent within the file. I.e. the above ought to look like the below instead (see also the code snippet in #8002 (comment)).

this.canvasFactory = new DOMCanvasFactory(); this.cachedCanvases = new CachedCanvases(this.canvasFactory);

Snuffleupagus · 2017-01-28T09:22:21Z

src/display/canvas.js

@@ -218,7 +212,7 @@ var CachedCanvases = (function CachedCanvasesClosure() {
        // reset canvas transform for emulated mozCurrentTransform, if needed
        canvasEntry.context.setTransform(1, 0, 0, 1, 0, 0);
      } else {
-        var canvas = createScratchCanvas(width, height);
+        var canvas = CanvasGraphics.DOMcanvasFactory.create(width, height);


In order to prevent errors, such as TypeError: CanvasGraphics.DOMcanvasFactory is undefined[Learn More] canvas.js:215:13, you'll need to pass in a canvasFactory instance when initializing CachedCanvases, i.e. change the "constructor" to

function CachedCanvases(canvasFactory) { this.canvasFactory = canvasFactory; this.cache = Object.create(null); } }

and then use this.canvasFactory.create(width, height) here.

I only ran gulp unittest and lint, but forgot to test the viewer. Thanks for pointing out, i will take care of that.

Please don't forget to also run the entire test-suite, using gulp test, to ensure that there's no rendering regressions.

Okay, thanks.

Snuffleupagus · 2017-01-28T09:27:53Z

src/display/dom_utils.js

@@ -33,6 +33,36 @@ var createValidAbsoluteUrl = sharedUtil.createValidAbsoluteUrl;

 var DEFAULT_LINK_REL = 'noopener noreferrer nofollow';

+function DOMCanvasFactory() {}


I'm not sure if using this the way that you do here below actually makes sense in the context of a factory!? I'd think that you may need to do something like this instead:

function DOMCanvasFactory() {} DOMCanvasFactory.prototype = { create: function DOMCanvasFactory_create(width, height) { var canvas = document.createElement('canvas'); canvas.width = width || 0; canvas.height = height || 0; return canvas; }, reset: function DOMCanvasFactory_reset(canvas, width, height) { if (!canvas) { throw new Error('Canvas not found'); } canvas.width = width; canvas.height = height; }, destroy: function DOMCanvasFactory_destroy(canvas) { if (!canvas) { throw new Error('Canvas not found'); } // Zeroing the width and height causes Firefox to release graphics // resources immediately, which can greatly reduce memory consumption. canvas.width = 0; canvas.height = 0; canvas = null; } };

Sorry, i think i was little messed up with constructor function pattern. I will change it.

Snuffleupagus

This is looking much better now!
Edit: It seems that this patch causes a lot of movement/failures in some of the tests, and I think I understand why. Please see the inline comment below.

However, I still think that there's one crucial thing left to do here. The way that I interpret issue #7798 is that we need to make it possible for users of the API to provide their own custom CanvasFactory, for example one using Node.js, for this refactoring to be truly useful.
~~I obviously don't know what @yurydelendik had in mind, but I'd think you probably want something along the lines of the following diff (created on top of this patch):~~ Edit: Based on comments below, this isn't exactly what we want here; hence I'll remove the diff to not cause unnecessary confusion.

Snuffleupagus · 2017-01-30T11:49:11Z

src/display/api.js

@@ -1742,7 +1742,8 @@ var WorkerTransport = (function WorkerTransportClosure() {
            var size = width * height;
            var rgbaLength = size * 4;
            var buf = new Uint8Array(size * components);
-            var tmpCanvas = createScratchCanvas(width, height);
+            var canvasFactory = new DOMCanvasFactory();


This means that a new DOMCanvasFactory gets created for every JPEG image a document contains, which seem like the opposite of how you'd actually want a factory to be used.
See the general review comments for more regarding this subject.

Here is the conversation with @yurydelendik, when i am trying to solve this issue. I think i got confused of passing canvasFactory via CanvasGraphics. I will fix it. Thanks for clearing.

I think we need to keep this part without change as well and add assert(typeof document !== 'undefined').

I'm thinking the canvasFactory must be a property of the render parameters. To protect core from calling browser decoding (which will fail anyway at setting image src), we need to pass a flag to evaluator that shows that browser decoding is disabled. I thought we do/did it, since pdf2svg example worked, but I cannot find this code now, so we need to pass this flag with getOperatorList requests.

It is still will be passed to the CanvasGraphics, but it will be specified at the RenderParameters. And if it is not specified, created at the render().

Snuffleupagus · 2017-01-30T11:51:21Z

src/display/font_loader.js

@@ -216,6 +216,8 @@ if (typeof PDFJSDev === 'undefined' || !PDFJSDev.test('MOZCENTRAL')) {

      var i, ii;

+      // CanvasFactory is not needed here, as it is used to determine if fonts
+      // is loaded.


Nit: I think are loaded. is better here, since the word "fonts" on the previous line is plural.

Oh yes, thanks for catch. I will fix it.

Snuffleupagus · 2017-01-30T12:52:29Z

src/display/dom_utils.js

+  create: function DOMCanvasFactory_create(width, height) {
+    var canvas = document.createElement('canvas');
+    canvas.width = width || 0;
+    canvas.height = height || 0;


Letting the width/height default to zero is breaking the get isFontSubpixelAAEnabled() method, with lots of test failures as a result. I think that you need to do e.g. this instead:

if (typeof width !== 'undefined') { canvas.width = width; } if (typeof height !== 'undefined') { canvas.height = height; }

Okay, Thanks. I will do it this way.

actually, the code at isFontSubpixelAAEnabled was just creating canvas with default size, we just need to remove || 0 or typeof width !== 'undefined' and specify needed canvas size at the caller (isFontSubpixelAAEnabled) e.g. 200x100 or 50x10

Then i think we should remove typeof width !== 'undefined', as || 0 is little bit more clean.

Edit: @yurydelendik, or we can remove both as we are adding assert for width and height parameters.

yes, meant to say need to remove || 0 and typeof width !== 'undefined'

yurydelendik · 2017-01-30T14:06:32Z

src/display/canvas.js

@@ -1454,7 +1450,7 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    get isFontSubpixelAAEnabled() {
      // Checks if anti-aliasing is enabled when scaled text is painted.
      // On Windows GDI scaled fonts looks bad.
-      var ctx = document.createElement('canvas').getContext('2d');
+      var ctx = this.canvasFactory.create().getContext('2d');


create canvas is size (10, 10) here

got it, thanks

yurydelendik · 2017-01-30T14:09:20Z

src/display/dom_utils.js

+  },
+
+  reset: function DOMCanvasFactory_reset(canvas, width, height) {
+    if (!canvas) {


Use assert(canvas, 'canvas is not specifed') here (and below)

add asserts (here and above) for the width and height parameters: assert(width > 0 && height > 0, 'invalid canvas size')

timvandermeij

This patch looks good to me after addressing these comments, but I'll leave the final review for others that know this particular code a bit better than me.

timvandermeij · 2017-02-01T23:07:19Z

src/display/api.js

@@ -1742,7 +1751,10 @@ var WorkerTransport = (function WorkerTransportClosure() {
            var size = width * height;
            var rgbaLength = size * 4;
            var buf = new Uint8Array(size * components);
-            var tmpCanvas = createScratchCanvas(width, height);
+            // make sure that this code is not executing in node.js.


Make this comment a bit more specific, i.e., why is it bad if this runs with Node.js?

Okay, thanks. I will do it.

timvandermeij · 2017-02-01T23:07:40Z

src/display/api.js

-            var tmpCanvas = createScratchCanvas(width, height);
+            // make sure that this code is not executing in node.js.
+            assert(typeof document !== 'undefined');
+            var canvasFactory = new DOMCanvasFactory();


Can't we use this.canvasFactory (as set above) here? If so, we can remove this line and use var tmpCanvas = this.canvasFactory.create(width, height); in the line below.

We probable need to make it var tmpCanvas = document.createElement('canvas'); tmpCanvas.width =...

yurydelendik · 2017-02-01T23:17:04Z

src/display/canvas.js

@@ -477,7 +473,8 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    this.smaskStack = [];
    this.smaskCounter = 0;
    this.tempSMask = null;
-    this.cachedCanvases = new CachedCanvases();
+    this.canvasFactory = canvasFactory || new DOMCanvasFactory();


canvasFactory parameter must be always specified (starting at render() method). can you also move canvasFactory before imageLayer in the parameters list.

yurydelendik · 2017-02-01T23:17:11Z

src/display/dom_utils.js

+    canvas.width = 0;
+    canvas.height = 0;
+
+    canvas = null;


remove canvas = null;

Also, this method must be used in the CachedCanvases.clear

yurydelendik · 2017-02-01T23:20:07Z

src/display/dom_utils.js

+DOMCanvasFactory.prototype = {
+  create: function DOMCanvasFactory_create(width, height) {
+    var canvas = document.createElement('canvas');
+    assert(width > 0 && height > 0, 'invalid canvas size');


move assert as a first line of the function

yurydelendik · 2017-02-01T23:24:28Z

src/display/api.js

@@ -132,6 +133,8 @@ if (typeof PDFJSDev !== 'undefined' &&
 * @property {string} docBaseUrl - (optional) The base URL of the document,
 *   used when attempting to recover valid absolute URLs for annotations, and
 *   outline items, that (incorrectly) only specify relative URLs.
+ * @property {Object} canvasFactory - (optional) The factory that will be used


Move canvasFactory from here and add to the RenderParameters below. We don't need to store canvas factory on the transport but rather than on a rendering task.

yurydelendik · 2017-02-01T23:26:29Z

src/display/api.js

-            var tmpCanvas = createScratchCanvas(width, height);
+            // make sure that this code is not executing in node.js.
+            assert(typeof document !== 'undefined');
+            var canvasFactory = new DOMCanvasFactory();


We probable need to make it var tmpCanvas = document.createElement('canvas'); tmpCanvas.width =...

Snuffleupagus

Sorry about bad advice previously!
Looking through the latest patch, it seems that a couple of things should probably be reverted/changed based on #8002 (comment) and #8002 (comment), so I've added inline comments where applicable.

Snuffleupagus · 2017-02-03T13:15:12Z

src/display/api.js

@@ -245,6 +246,7 @@ function getDocument(src, pdfDataRangeTransport,
  }

  params.rangeChunkSize = params.rangeChunkSize || DEFAULT_RANGE_CHUNK_SIZE;
+  var canvasFactory = params.canvasFactory || new DOMCanvasFactory();


Based on #8002 (comment), this line should be removed.

Snuffleupagus · 2017-02-03T13:15:55Z

src/display/api.js

@@ -262,7 +264,8 @@ function getDocument(src, pdfDataRangeTransport,
        throw new Error('Loading aborted');
      }
      var messageHandler = new MessageHandler(docId, workerId, worker.port);
-      var transport = new WorkerTransport(messageHandler, task, rangeTransport);
+      var transport = new WorkerTransport(messageHandler, task, rangeTransport,
+                                          canvasFactory);


Based on #8002 (comment), this parameter should be removed.

Snuffleupagus · 2017-02-03T13:16:25Z

src/display/api.js

@@ -720,6 +726,7 @@ var PDFPageProxy = (function PDFPageProxyClosure() {
    this.transport = transport;
    this.stats = new StatTimer();
    this.stats.enabled = getDefaultSetting('enableStats');
+    this.canvasFactory = transport.canvasFactory;


Based on #8002 (comment). this line should be removed.

Snuffleupagus · 2017-02-03T13:17:59Z

src/display/api.js

@@ -1410,10 +1418,12 @@ var PDFWorker = (function PDFWorkerClosure() {
 * @ignore
 */
 var WorkerTransport = (function WorkerTransportClosure() {
-  function WorkerTransport(messageHandler, loadingTask, pdfDataRangeTransport) {
+  function WorkerTransport(messageHandler, loadingTask, pdfDataRangeTransport,
+                           canvasFactory) {


Based on #8002 (comment), this parameter should be removed.

Snuffleupagus · 2017-02-03T13:18:11Z

src/display/api.js

    this.messageHandler = messageHandler;
    this.loadingTask = loadingTask;
    this.pdfDataRangeTransport = pdfDataRangeTransport;
+    this.canvasFactory = canvasFactory;


Based on #8002 (comment), this line should be removed.

Snuffleupagus · 2017-02-03T13:20:08Z

src/display/api.js

@@ -834,7 +841,8 @@ var PDFPageProxy = (function PDFPageProxyClosure() {
                                                      this.objs,
                                                      this.commonObjs,
                                                      intentState.operatorList,
-                                                      this.pageNumber);
+                                                      this.pageNumber,
+                                                      this.canvasFactory);


Based on #8002 (comment), this line should be replaced by canvasFactory, with

var canvasFactory = params.canvasFactory || new DOMCanvasFactory();

defined at line https://github.com/mozilla/pdf.js/pull/8002/files#diff-0ecad279ed2252e3eb47a4d96ec1463cR815 above.

Snuffleupagus · 2017-02-03T13:21:05Z

src/display/canvas.js

@@ -466,6 +459,7 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
    this.xobjs = null;
    this.commonObjs = commonObjs;
    this.objs = objs;
+    this.canvasFactory = canvasFactory || new DOMCanvasFactory();


Based on #8002 (comment), this line should simply read

this.canvasFactory = canvasFactory;

Snuffleupagus · 2017-02-03T13:22:12Z

src/display/canvas.js

@@ -456,7 +448,8 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
  // Defines the number of steps before checking the execution time
  var EXECUTION_STEPS = 10;

-  function CanvasGraphics(canvasCtx, commonObjs, objs, imageLayer) {
+  function CanvasGraphics(canvasCtx, commonObjs, objs, imageLayer,
+                          canvasFactory) {


Based on #8002 (comment), please change the order of imageLayer and canvasFactory here.

Snuffleupagus · 2017-02-03T13:23:26Z

src/display/api.js

@@ -2068,7 +2084,8 @@ var InternalRenderTask = (function InternalRenderTaskClosure() {

      var params = this.params;
      this.gfx = new CanvasGraphics(params.canvasContext, this.commonObjs,
-                                    this.objs, params.imageLayer);
+                                    this.objs, params.imageLayer,
+                                    this.canvasFactory);


Based on #8002 (comment), please change the order of params.imageLayer and this.canvasFactory here.

Snuffleupagus · 2017-02-03T13:28:04Z

src/display/api.js

-            var tmpCanvas = createScratchCanvas(width, height);
+            // make sure that this code is not executing in node.js, as
+            // it's using DOM image, and there is no library to support that.
+            assert(typeof document !== 'undefined');


Looking at the entire JpegDecode code here, please see https://github.com/mozilla/pdf.js/blob/master/src/display/api.js#L1724-L1769, the use of assert seems slightly out of place here.

@yurydelendik I'd think it'd be more in line with the rest of this code-block if we did

if (typeof document === 'undefined') { // Make sure that this code is not executed in Node.js, as // it's using DOM image, and there is no library to support that. return Promise.reject(new Error('"document" is not defined.')); }

placed at line https://github.com/mozilla/pdf.js/blob/master/src/display/api.js#L1728 instead.

Either way works for me - it's just to make it easier to catch a problem. We can even move assert/reject at the top of the function.

Snuffleupagus · 2017-02-04T13:26:37Z

src/display/api.js

 var Metadata = displayMetadata.Metadata;
 var getDefaultSetting = displayDOMUtils.getDefaultSetting;
+var DOMCanvasFactory = displayDOMUtils.DOMCanvasFactory;
+var assert = sharedUtil.assert;


Remove this line, since assert is now unused (which is also why linting failed).

Snuffleupagus · 2017-02-04T13:27:30Z

src/display/api.js

@@ -1727,6 +1733,12 @@ var WorkerTransport = (function WorkerTransportClosure() {
          return Promise.reject(new Error('Worker was destroyed'));
        }

+        if (typeof document === 'undefined') {
+          // make sure that this code is not executing in node.js, as


Nit: Please remember to use uppercase letters at the beginning of sentences in comments, i.e. // Make sure ... instead.

yurydelendik

Looks good to me with comments below addressed and tests are passing.

yurydelendik · 2017-02-04T14:30:09Z

src/display/dom_utils.js

+    var canvas = document.createElement('canvas');
+    canvas.width = width;
+    canvas.height = height;
+


nit: we can remove a empty line here.

yurydelendik · 2017-02-04T14:30:33Z

src/display/dom_utils.js

+    return canvas;
+  },
+
+  reset: function DOMCanvasFactory_reset(canvas, width, height) {


This function must be used from CachedCanvases

yurydelendik · 2017-02-04T14:33:10Z

src/display/font_loader.js

@@ -216,6 +216,8 @@ if (typeof PDFJSDev === 'undefined' || !PDFJSDev.test('MOZCENTRAL')) {

      var i, ii;

+      // CanvasFactory is not needed here, as it is used to determine if fonts


Here and below, replace "CanvasFractory is not needed here, as it is" to something like "The temporary canvas is"

previous concerns are addressed

Earlier comments have now been addressed.

Snuffleupagus · 2017-02-05T14:44:17Z

src/display/canvas.js

@@ -213,12 +207,11 @@ var CachedCanvases = (function CachedCanvasesClosure() {
      var canvasEntry;
      if (this.cache[id] !== undefined) {
        canvasEntry = this.cache[id];
-        canvasEntry.canvas.width = width;
-        canvasEntry.canvas.height = height;
+        this.canvasFactory.reset(canvasEntry, width, height);


Are you sure that this actually works, since I believe that you need to use canvasEntry.canvas instead (compare with e.g. the destroy(...) call below).

Oh, okay. I missed something. Sorry

Fixes extra canvas create calls. Fixes unnecessary call of `new DOMCanvasFactory`. Fixes undefined error of DOMCanvasFactory. Fixes failures in some of the tests. Fixes expected behaviour. Remove unused vars.

pdfjsbot · 2017-02-06T11:38:25Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/cc716af9f1aef5e/output.txt

pdfjsbot · 2017-02-06T11:38:25Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/2eac7c0ccb9ad21/output.txt

pdfjsbot · 2017-02-06T12:00:45Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/2eac7c0ccb9ad21/output.txt

Total script time: 22.33 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

pdfjsbot · 2017-02-06T12:04:22Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/cc716af9f1aef5e/output.txt

Total script time: 25.96 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

yurydelendik · 2017-02-06T13:47:36Z

Thank you for the patch.

It's very good step, but I think to make it work we need to change a factory to return {canvas, context} pair instead to make it work for node's canvas. @mukulmishra18, would you like to follow up?

mukulmishra18 · 2017-02-06T14:45:27Z

@yurydelendik, @Snuffleupagus, @timvandermeij thanks for your reviews and help. You all are very supportive and cooperative, i learned a lot from all of you. I definitely wants to continue, please guide me what i have to do.

yurydelendik · 2017-02-06T14:55:42Z

I definitely wants to continue, please guide me what i have to do.

Looking the the way node-canvas is create, we will need to create a new canvas and content on reset/resize. Let's just return create CanvasAndContextPair "structure" from create method and accept it in reset and destroy calls. CachedCanvases will/do store this structure. Notice that every time we create canvas we always create 2d-context after that, so we may do that in the factory.

[api-minor] Fix mozilla#7798: Refactor scratch canvas usage.

yurydelendik requested changes Jan 27, 2017

View reviewed changes

timvandermeij added the other label Jan 27, 2017

mukulmishra18 force-pushed the refactor-canvas branch from e6d80e8 to 85c6f74 Compare January 28, 2017 08:14

Snuffleupagus requested changes Jan 28, 2017

View reviewed changes

mukulmishra18 force-pushed the refactor-canvas branch from 85c6f74 to 1bd3108 Compare January 28, 2017 19:06

Snuffleupagus previously requested changes Jan 30, 2017

View reviewed changes

Snuffleupagus reviewed Jan 30, 2017

View reviewed changes

yurydelendik reviewed Jan 30, 2017

View reviewed changes

mukulmishra18 force-pushed the refactor-canvas branch from 1bd3108 to 0fce242 Compare February 1, 2017 18:49

timvandermeij reviewed Feb 1, 2017

View reviewed changes

yurydelendik reviewed Feb 1, 2017

View reviewed changes

yurydelendik requested changes Feb 1, 2017

View reviewed changes

mukulmishra18 force-pushed the refactor-canvas branch from 0fce242 to 02f1146 Compare February 2, 2017 08:11

Snuffleupagus reviewed Feb 3, 2017

View reviewed changes

mukulmishra18 force-pushed the refactor-canvas branch from 02f1146 to 4ed57dd Compare February 4, 2017 13:16

Snuffleupagus reviewed Feb 4, 2017

View reviewed changes

mukulmishra18 force-pushed the refactor-canvas branch from 4ed57dd to efb4978 Compare February 4, 2017 13:37

yurydelendik previously requested changes Feb 4, 2017

View reviewed changes

Snuffleupagus changed the title ~~Fix #7798: Refactor scratch canvas usage.~~ [api-minor] Fix #7798: Refactor scratch canvas usage. Feb 4, 2017

mukulmishra18 force-pushed the refactor-canvas branch from efb4978 to 6edee09 Compare February 5, 2017 14:39

Snuffleupagus reviewed Feb 5, 2017

View reviewed changes

Fix mozilla#7798: Refactor scratch canvas usage.

3281763

Fixes extra canvas create calls. Fixes unnecessary call of `new DOMCanvasFactory`. Fixes undefined error of DOMCanvasFactory. Fixes failures in some of the tests. Fixes expected behaviour. Remove unused vars.

mukulmishra18 force-pushed the refactor-canvas branch from 6edee09 to 3281763 Compare February 5, 2017 14:51

yurydelendik merged commit d842c9c into mozilla:master Feb 6, 2017

mukulmishra18 deleted the refactor-canvas branch September 19, 2017 08:21

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8002 from mukulmishra18/refactor-canvas

edd69ff

[api-minor] Fix mozilla#7798: Refactor scratch canvas usage.

		@@ -216,7 +218,8 @@ if (typeof PDFJSDev === 'undefined' \|\| !PDFJSDev.test('MOZCENTRAL')) {

		var i, ii;

		var canvas = document.createElement('canvas');

		var Metadata = displayMetadata.Metadata;
		var getDefaultSetting = displayDOMUtils.getDefaultSetting;

		@@ -33,6 +33,36 @@ var createValidAbsoluteUrl = sharedUtil.createValidAbsoluteUrl;

		var DEFAULT_LINK_REL = 'noopener noreferrer nofollow';

		function DOMCanvasFactory() {}

		@@ -216,6 +216,8 @@ if (typeof PDFJSDev === 'undefined' \|\| !PDFJSDev.test('MOZCENTRAL')) {

		var i, ii;

		// CanvasFactory is not needed here, as it is used to determine if fonts

[api-minor] Fix #7798: Refactor scratch canvas usage. #8002

[api-minor] Fix #7798: Refactor scratch canvas usage. #8002

Conversation

mukulmishra18 commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mukulmishra18 Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Feb 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Jan 28, 2017 •

edited

Loading

Snuffleupagus Jan 28, 2017 •

edited

Loading

Snuffleupagus Jan 28, 2017 •

edited

Loading

Snuffleupagus Jan 28, 2017 •

edited

Loading

Snuffleupagus Jan 28, 2017 •

edited

Loading

Snuffleupagus Jan 28, 2017 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

yurydelendik Jan 30, 2017 •

edited

Loading

Snuffleupagus Jan 30, 2017 •

edited

Loading

mukulmishra18 Jan 30, 2017 •

edited

Loading

Snuffleupagus Feb 3, 2017 •

edited

Loading

Snuffleupagus Feb 5, 2017 •

edited

Loading