Refactoring of printing code and mozPrintCallback polyfill #7697

yurydelendik · 2016-10-07T13:27:46Z

Two things happens: moves all print related code to the single file and makes the polyfill as a first-class citizen. My finding says that Firefox now rasterizes mozPrintCallback output which aligns it with other browsers by functionality. Another finding (and lots of bugs confirm) that mozPrintCallback will not print using font that were not used during viewing. So migrating generic/demo viewer to not use mozPrintCallback will be good first step to produce consistent results across browsers. Converting canvas to PNG saves memory and improves print quality for Chrome.

Snuffleupagus · 2016-10-07T14:54:18Z

web/pdf_print_service.js

+      return pdfPage.render(renderContext).promise;
+    }).then(function() {
+      img.src = canvas.toDataURL();
+      wrapper.appendChild(img);


To try and save memory you probably want to clear the canvas explicitly as soon as it's no longer needed, as is done e.g. in

pdf.js/web/pdf_page_view.js

Lines 170 to 174 in f8bd3d4

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

// resources immediately, which can greatly reduce memory consumption.

this.canvas.width = 0;

this.canvas.height = 0;

delete this.canvas;

.

Fixed: using one canvas for all pages and disposing it at the end.

Snuffleupagus · 2016-10-07T15:00:29Z

web/viewer.html

+              <progress value="0" max="100"></progress>
+              <span class="relative-progress">0%</span>
+            </div>
+            <div class="row">


I think that you want class="buttonRow" instead here.

yurydelendik · 2016-10-07T15:11:41Z

I didn't decide about one thing:

remove "pageStyleSheet with A4 selector"-support (I'm not sure how it works)

yurydelendik · 2016-10-07T15:21:20Z

After reading the comment about bug 851441, I removed page hack for firefox print service in the second commit.

Snuffleupagus · 2016-10-07T15:36:09Z

web/app.js

-    }
+    // The beforePrint is a sync method and we need to know layout before
+    // returning from this method. Ensure that we can get sizes of the pages.
+    var alertNotReady = !this.pdfViewer.pageViewsReady;
    if (alertNotReady) {


At this point I don't think that you need the intermediate alertNotReady variable any more, so
if (!this.pdfViewer.pageViewsReady) { should suffice here.

Snuffleupagus · 2016-10-07T15:39:39Z

web/app.js

+    if (this.printing) {
+      this.printService.destroy();
+      this.printService = null;
+      this.printing = false;


Given the way that this.printing/this.printService is now used, we (basically) track the same state twice.
I'm wondering if we couldn't just get rid of the this.printing from this file and just check the new this.printService instead (possibly with !!this.printService where we actually want a boolean, such as e.g. in forceRendering)?

Fixed by adding "printing" getter (just in case if people use the property) or as in case above (for simplicity). Internally in app.js it's mostly used as printService

Snuffleupagus · 2016-10-07T15:40:43Z

web/pdf_print_service.js

+
+  function renderPage(pdfDocument, pageNumber, size, wrapper) {
+    if (!scratchCanvas) {
+       scratchCanvas = document.createElement('canvas');


Nit: indentation.

Rob--W · 2016-10-07T17:35:58Z

Why did you remove the mozPrintCallback polyfill? Now the code has two code paths, one where mozPrintCallback is available and one where it is not. How does this not affect printing behavior (or how does the change improve the behavior)?

yurydelendik · 2016-10-07T18:29:30Z

Why did you remove the mozPrintCallback polyfill?

code was just moved to the pdf_print_service.js

Now the code has two code paths, one where mozPrintCallback is available and one where it is not.

For simplicity, web/demo viewer will never use mozPrintCallback -- it is in bad shape now (see 1308259 and 1308579) and performs rasterization like we do, and yes, for the extension we continue using mozPrintCallback just not create a drastic change. If decision to improve/remove mozPrintCallback will be made we can either use firefox_print_callback in the pdf_print_callback or remove firefox_print_callback.

How does this not affect printing behavior (or how does the change improve the behavior)?

The changes are:

Firefox in the demo viewer prints the same way IE or Chrome and quality of output shall be a little bit better (due to png)
No style hacks in the Firefox extension code
Better readability of the printing logic within viewer
Ability to replace CANVAS/PNG with SVG output

@Rob--W, I hope I answered the questions. Thanks

timvandermeij

I did a quick pass through the code and these are my findings so far. In general I really like the direction of this patch, but this still requires more in-depth review and testing.

timvandermeij · 2016-10-07T21:06:07Z

web/firefox_print_service.js

+  var CSS_UNITS = uiUtils.CSS_UNITS;
+  var PDFPrintServiceFactory = app.PDFPrintServiceFactory;
+
+  function composePage(pdfDocument, pageNumber, size, printContainer) {


This is called renderPage in pdf_print_service.js. Can we name it renderPage here too for consistency?

Not really, I added a comments to pdf_print_service

timvandermeij · 2016-10-07T21:10:55Z

web/pdf_print_service.js

+      dispatchEvent('beforeprint');
+    } finally {
+      if (!activeService) {
+        console.error('Expected print service to the initialized.');


Nit: to be initialized

timvandermeij · 2016-10-07T21:12:10Z

web/pdf_print_service.js

+        throw new Error('The print service is created and active.');
+      }
+      activeService = new PDFPrintService(pdfDocument, pagesOverview,
+                                         printContainer);


Nit: add one more space

timvandermeij · 2016-10-07T21:13:20Z

web/pdf_viewer.js

+     * Returns sizes of the pages.
+     * @returns {Array} Array of objects with width/height fields.
+     */
+    getPagesOverview: function () {


Would getPageSizes perhaps be a better name, or do you want to keep it general to add more properties later on?

The latter, but I don't want to think now what to add :)

timvandermeij · 2016-10-07T21:16:26Z

I think that this supersedes #7408 and fixes #3783 as well, judging by the code changes, but I did not check that yet.

Rob--W

This looks really good. I have found a few minor issues, commented below.

Now the mozPrintCallback polyfill is integrated, it does not need to be so generic any more and the implementation can be simplified. But I can do that since I exactly know the purpose of each line in the polyfill.

Rob--W · 2016-10-07T22:43:21Z

web/pdf_print_service.js

+      return pdfPage.render(renderContext).promise;
+    }).then(function() {
+      img.src = scratchCanvas.toDataURL();
+      wrapper.appendChild(img);


I think that you have to wait until img.onload is fired.

return new Promise(function(resolve, reject) { img.onload = resolve; img.onerror = reject; });

Some food for thought: Would canvas.toBlob() over toDataURL improve memory usage or performance? In theory, after painting the browser does not need to keep the very long data URL around, which leads to a reduced memory usage. Here is a code sample: https://developer.mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/toBlob#Getting_a_file_representing_the_canvas

Fixed and improved.

Rob--W · 2016-10-07T22:55:28Z

web/pdf_print_service.js

+        scratchCanvas = null;
+      }
+      ensureOverlay().then(function () {
+        OverlayManager.close('printServiceOverlay');


When I send a print request and close the print dialog, an error is thrown here:

Uncaught (in promise) Error: The overlay is currently not active.(…) overlay_manager.js:117
(anonymous function) @ overlay_manager.js:117
overlayManagerClose @ overlay_manager.js:113
(anonymous function) @ pdf_print_service.js:140

I think that the fact that destroy() is called twice causes this issue.

abort below calls .destroy()

In app.js, the afterprint event handler calls destroy() as well.

check to not perform double close() added

Rob--W · 2016-10-07T22:57:59Z

web/pdf_viewer.js

@@ -162,6 +162,13 @@ var PDFViewer = (function pdfViewer() {
    },

    /**
+     * @returns {boolean} true if all page views are initialized.


What is a "page view"? The data structure behind a PDF page? A canvas element?

Comment fixed.

Rob--W · 2016-10-07T22:59:17Z

web/viewer.html

+        <div id="printServiceOverlay" class="container hidden">
+          <div class="dialog">
+            <div class="row">
+              <span>Preparing document for printing...</span>


The localization comment disappeared. Don't we want to localize this any more?

Good find. I really think we should localize this and I think it's easier now that it's part of viewer.html. Otherwise we should add back the comment.

Comment does not work with proprocessor

Rob--W · 2016-10-07T23:00:48Z

web/pdf_print_service.js

+        intent: 'print'
+      };
+      return pdfPage.render(renderContext).promise;
+    }).then(function() {


Need an if-check here to see if the print job is still active.

Rob--W · 2016-10-07T23:01:42Z

web/pdf_print_service.js

+
+    renderPages: function () {
+      var pageCount = this.pagesOverview.length;
+      var renderNextPage = function (resolve, reject) {


Need an if-check to see if the print request is still active, and exit if it is not.

Snuffleupagus · 2016-10-08T08:22:17Z

web/pdf_print_service.js

+    var progressPerc = progressContainer.querySelector('.relative-progress');
+    progressBar.value = progress;
+    progressPerc.textContent = progress + '%';
+    progressContainer.onclick = abort;


As mentioned on IRC, I think that the onclick handler should be attached to the printCancel button rather than the entire overlay container.

yurydelendik · 2016-10-08T12:58:26Z

Nits addressed and additional checks are added.

/botio-linux preview

Snuffleupagus

r=me; but let's have Rob and Tim sign off on this too before merging.
In my testing this works great, and the re-factoring improves readability of the code quite a bit!

Rob--W · 2016-10-10T20:24:51Z

Looks good from the users perspective.

Tested in Firefox 49 on Linux
Tested in Firefox 49 on macOS
Tested in Chromium 53 on Linux
Tested as a Chrome extension in Chrome 54 on Linux
Test in Safari 10 on macOS
Test as a Firefox addon in Firefox
Test in IE (version?)
Test in Edge on Windows

The unticked checkboxes are still a TODO. Any volunteers? ;)

Tested scenarios:

Ctrl-P, wait, close platform's print dialog.
Ctrl-P, Esc
Ctrl-P, Ctrl-P, Esc
Click Print button
Ctrl-P/Print button, click Cancel button in PDF.js dialog.

Note: When I use Esc while the PDF is being prepared, in Firefox the following error appears after a few seconds. I think that the dialog is already closed by the esc keydown handler in the OverlayManager, when the .destroy() call's overlay promise resolves and closes the dialog.

Error: The overlay is currently not active.viewer.js:333:15
(put breakpoint + printed new Error().stack to get the following stack trace)
overlayManagerClose/<@http://107.21.233.14:8877/be6471bdd1701a6/web/viewer.js:333:9
overlayManagerClose@http://107.21.233.14:8877/be6471bdd1701a6/web/viewer.js:329:12
PDFPrintService.prototype.destroy/<@http://107.21.233.14:8877/be6471bdd1701a6/web/viewer.js:9543:9

timvandermeij · 2016-10-10T21:47:04Z

Tomorrow I'll have access to a computer with Windows, so I can take care of the remaining checkboxes and review this as well so we can land it.

@yurydelendik Could you look into the error mentioned above? The OverlayManager does have a key binding for the Esc key: https://github.com/mozilla/pdf.js/blob/master/web/overlay_manager.js#L135

timvandermeij · 2016-10-10T22:03:43Z

More of a note to myself for testing, but here is a list of issues/PRs possibly affected by these patches:

yurydelendik · 2016-10-11T13:55:45Z

Could you look into the error mentioned above?

Added guard against closing closed overlay and removed esc handling from print service.

timvandermeij · 2016-10-11T14:35:59Z

web/firefox_print_service.js

+      }, this);
+      if (!hasEqualPageSizes) {
+        console.warn('Not all pages have the same size. The printed ' +
+                      'result may be incorrect!');


Nit: remove one space

timvandermeij · 2016-10-11T15:00:16Z

web/pdf_print_service.js

+      if (scratchCanvas) {
+        scratchCanvas.width = scratchCanvas.height = 0;
+        scratchCanvas = null;
+      }


Shouldn't we clear this.wrappers and the associated divs here as well to save memory?

timvandermeij · 2016-10-11T15:12:27Z

/botio-linux preview

pdfjsbot · 2016-10-11T15:12:28Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-10-11T15:13:29Z

From: Bot.io (Linux)

Success

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

Total script time: 1.02 mins

Published

timvandermeij · 2016-10-11T19:23:13Z

Awesome work! I tested this in all major browsers and found no issues.

webmobiletechie · 2017-02-03T12:19:57Z

I am facing similar issues while trying to print huge PDFs (that are being rendered using PDFJS) on IE11. Upgrading to latest version of PDFJS hasn't helped either. The above conversation hints that the issue may be already fixed. Where can I get the sample code? Any pointers?

Refactoring of printing code and mozPrintCallback polyfill

Snuffleupagus reviewed Oct 7, 2016

View reviewed changes

yurydelendik force-pushed the print-v2 branch from a31e087 to 02845f4 Compare October 7, 2016 15:08

yurydelendik force-pushed the print-v2 branch from 02845f4 to c00129f Compare October 7, 2016 15:19

Snuffleupagus reviewed Oct 7, 2016

View reviewed changes

yurydelendik force-pushed the print-v2 branch from c00129f to 32913db Compare October 7, 2016 15:47

Rob--W added the printing label Oct 7, 2016

timvandermeij reviewed Oct 7, 2016

View reviewed changes

Rob--W reviewed Oct 7, 2016

View reviewed changes

Snuffleupagus reviewed Oct 8, 2016

View reviewed changes

Moves mozPrintCallback specific code to firefox_printservice.js

24a7a58

yurydelendik force-pushed the print-v2 branch from 32913db to 5268933 Compare October 8, 2016 12:55

Rob--W mentioned this pull request Oct 9, 2016

Printing from subframe does not work in Chrome 54 #7706

Closed

Snuffleupagus approved these changes Oct 10, 2016

View reviewed changes

timvandermeij assigned Rob--W, timvandermeij and Snuffleupagus Oct 10, 2016

yurydelendik force-pushed the print-v2 branch from 5268933 to 55c3b13 Compare October 11, 2016 13:54

timvandermeij reviewed Oct 11, 2016

View reviewed changes

yurydelendik added 2 commits October 11, 2016 10:08

Removes mozPrintCallback polyfill, converts canvas to PNG.

c09f634

Localization of the print dialog.

1a056ca

yurydelendik force-pushed the print-v2 branch from 55c3b13 to 1a056ca Compare October 11, 2016 15:08

timvandermeij approved these changes Oct 11, 2016

View reviewed changes

timvandermeij merged commit fb5aa58 into mozilla:master Oct 11, 2016

This was referenced Oct 11, 2016

Update print dialog to reuse existing dialog styles. #7408

Closed

Style print dialog #3783

Closed

Localize / translate all strings #6333

Closed

Rob--W mentioned this pull request Oct 13, 2016

Add work-around to get printing in OOP frames to work in Chrome #7721

Closed

timvandermeij mentioned this pull request Nov 7, 2016

Enabling to add watermarks on pdf.js pages #7783

Closed

Rob--W mentioned this pull request Jan 22, 2017

Pdf not printing properly in chrome browser. #7963

Closed

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

Merge pull request mozilla#7697 from yurydelendik/print-v2

f8f2ac2

Refactoring of printing code and mozPrintCallback polyfill

Snuffleupagus mentioned this pull request Feb 8, 2019

Stop using mozPrintCallback in generic builds #10528

Closed

Snuffleupagus mentioned this pull request Nov 19, 2019

Ensure that attempting to print with disableAutoFetch set will fail gracefully (issue 11339) #11346

Merged

Snuffleupagus mentioned this pull request Apr 23, 2022

Remove unnecessary print CSS rules #14829

Merged

	// Zeroing the width and height causes Firefox to release graphics
	// resources immediately, which can greatly reduce memory consumption.
	this.canvas.width = 0;
	this.canvas.height = 0;
	delete this.canvas;

Refactoring of printing code and mozPrintCallback polyfill #7697

Refactoring of printing code and mozPrintCallback polyfill #7697

Conversation

yurydelendik commented Oct 7, 2016 • 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

yurydelendik commented Oct 7, 2016 • edited Loading

yurydelendik commented Oct 7, 2016

Choose a reason for hiding this comment

Snuffleupagus Oct 7, 2016 • 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

Rob--W commented Oct 7, 2016

yurydelendik commented Oct 7, 2016 • edited Loading

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

timvandermeij commented Oct 7, 2016

Rob--W left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W Oct 7, 2016 • 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 Oct 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob--W Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented Oct 8, 2016

Snuffleupagus left a comment

Choose a reason for hiding this comment

Rob--W commented Oct 10, 2016 • edited by timvandermeij Loading

timvandermeij commented Oct 10, 2016 • edited Loading

timvandermeij commented Oct 10, 2016 • edited Loading

yurydelendik commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timvandermeij commented Oct 11, 2016

pdfjsbot commented Oct 11, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 11, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Oct 11, 2016

webmobiletechie commented Feb 3, 2017

yurydelendik commented Oct 7, 2016 •

edited

Loading

yurydelendik commented Oct 7, 2016 •

edited

Loading

Snuffleupagus Oct 7, 2016 •

edited

Loading

yurydelendik commented Oct 7, 2016 •

edited

Loading

Rob--W Oct 7, 2016 •

edited

Loading

timvandermeij Oct 8, 2016 •

edited

Loading

Rob--W Oct 7, 2016 •

edited

Loading

Rob--W commented Oct 10, 2016 •

edited by timvandermeij

Loading

timvandermeij commented Oct 10, 2016 •

edited

Loading

timvandermeij commented Oct 10, 2016 •

edited

Loading