flypig.co.uk

Good morning! It's Day 100 of my gecko dev diary. As I mentioned yesterday, that feels like it might be a milestone, although it's Day 128 that I'm holding out for (which, if things continue as they are, will be on 2nd January).

To celebrate this centenary of sorts, sailfish-gecko artist-in-residence Thigg has generated another wonderful image for us! This is gecko accepting pull requests: look at the gecko absorbing all those wholesome changes! Thank you Thigg for another wonderful and colourful way to brighten up the blog!
 
Yesterday we already started looking at the printing pipeline and discovered that the trigger we've been using up to now, which involves calling Downloads.createDownload() with a saver: "pdf" parameter is no longer going to work in ESR 91. That entire execution path has been removed.

We could patch it back in again, but I'd expect there to be a better way available now which we should be using instead.

In ESR 78 the code to create a savable PDF file of a page eventually ends up creating a DownloadPDFSaver which can be found in DownloadCore.jsm. This class has been removed, but that doesn't mean we can't use it for inspiration.

A key part of this class, the bit that triggers the printing process, appears to be this:

    this._webBrowserPrint = win.getInterface(Ci.nsIWebBrowserPrint);

    try {
      await new Promise((resolve, reject) => {
        this._webBrowserPrint.print(printSettings, {
          onStateChange(webProgress, request, stateFlags, status) {
            if (stateFlags & Ci.nsIWebProgressListener.STATE_STOP) {
              if (!Components.isSuccessCode(status)) {
                reject(new DownloadError({ result: status, inferCause: true }));
              } else {
                resolve();
              }
            }
          },
[...]

I'm encouraged by the fact that, although the DownloadPDFSaver class has been removed, the nsIWebBrowserPrint interface is alive and well and living in nsIWebBrowserPrint.idl. There's also a C++ implementation of it.

But rather than try to reverse engineer this interface, I'm going to try to figure out how this gets called in existing Firefox implementations. That might provide us some inspiration for how we're supposed to do it here.

Digging through all of the files, the most encouraging seems to be gecko-dev/toolkit/components/printing/content/print.js which contains plenty of references to PDF generation. So my task for today will be to read through this file and try to digest it.

[...]

Having read through print.js in some detail I'm not sure this is where I need to be. It provides the print dialogue box and while it does allow the page to be printed out as a PDF, this is all a bit heavyweight what we need.

I've also been carefully reading through the DownloadCore code and in particular the DownloadPDFSaver class. In practice there's actually not as much going on there as I'd originally thought. There's quite a bit of indirection as a result of the serializable constructors, but in practice our sailfish-browser path is just creating an instance of DownloadPDFSaver and using it as the "saver" entry in a Download object.

But what does the DownloadPDFSaver do? It essentially has execute and cancel methods. The former just sets up the print settings for a standard PDF printout, after which it extracts the nsIWebBrowserPrint interface from the current window and executes the print() method on it. The latter is even simpler: it just calls cancel on the same interface.

Consequently if we were to implement a process for printing out the page it would have to go through exactly the same steps. So we may as well use what's there.

However, what I'm thinking is that rather than revert the revert that removed the changes, essentially forward-porting them to the newer code with a patch, it would be better to just drag this functionality into the EmbedLite code, inside the EmbedliteDownloadManager in particular.

We'll still be using the standard print interface, we just won't have to rely on the expectation of patches applying cleanly to future versions of what — it turns out — is not always the most stable codebase.

The only downside to this approach is that I don't yet know whether the underlying printing mechanisms are going to work. That means I'll be making a lot of changes without being able to adequately test them.

Given that, as a first step I plan to reapply the reverted changes, get the underlying print mechanism working and only then move the changes into EmbedliteDownloadManager.

This seems like a good plan to me.

So, for now I've reapplied the changes to reintroduce DownloadPDFSaver. The changes really aren't so big and, in fact, only apply to the single file DownloadCore.js. I could rebuild and reinstall the package to test it now, but given that there are only a few changes to a single file, all of which are on the JavaScript side, I think I'll have a go at applying them directly on my development phone.

The file I'm changing exists in omni.ja so I can use my script to pack and repack the file.

$ ./omni.sh unpack
Omni action: unpack
Unpacking from: /usr/lib64/xulrunner-qt5-91.9.1
Unpacking to:   ./omni

$ find . -iname "DownloadCore.jsm"
./omni/modules/DownloadCore.jsm
$ vim ./omni/modules/DownloadCore.jsm
$ ./omni.sh pack
Omni action: pack
Packing from: ./omni
Packing to:   /usr/lib64/xulrunner-qt5-91.9.1

$ EMBED_CONSOLE=1 sailfish-browser

Having applied these changes, now when I select the "Save web page as PDF" option I get a new error:

EmbedliteDownloadManager error: this._webBrowserPrint.print is not a function
JavaScript error: , line 0: uncaught exception: Object
CONSOLE message:
[JavaScript Error: "uncaught exception: Object"]

That's progress of sorts! The this._webBrowserPrint object that's causing this new error is accessed like this:

    this._webBrowserPrint = win.getInterface(Ci.nsIWebBrowserPrint);

So the object should be an instance of nsIWebBrowserPrint. Checking the interface definition file I can see there is a print() method defined like this:

  /**
   * Print the specified DOM window
   *
   * @param aThePrintSettings - Printer Settings for the print job, if
   *                            aThePrintSettings is null then the global PS
   *                            will be used.
   * @param aWPListener - is updated during the print
   * @return void
   */
  [noscript] void print(in nsIPrintSettings aThePrintSettings,
                        in nsIWebProgressListener aWPListener);

This is a slight change from the ESR 78 signature which looked like this:

  void print(in nsIPrintSettings aThePrintSettings,
             in nsIWebProgressListener aWPListener);

This change could be what's causing the problems. A quick run of git blame shows that the [noscript] tags were added in order to block access to these calls from JavaScript, so that would definitely explain what's going wrong.

$ git log -1 fbca1f9c2385b
commit fbca1f9c2385be0178d4dff3debef2728b049d74
Author: Jonathan Watt <jwatt@jwatt.org>
Date:   Sun Jul 12 16:39:30 2020 +0000

    Bug 1652337. Prevent script from calling nsIWebBrowserPrint.print().
    r=bobowen
    
    Differential Revision: https://phabricator.services.mozilla.com/D83264

Here's what the bug description explains about this:
 

There is only one "caller" left in the dead code that should be removed by bug 1641805. I don't have the time to figure out a proper patch to that bug right now, but that shouldn't stop us marking this API [noscript] to make reviews of changes to its implementation easier to review.

Well, we've just added that caller back in. And even if we make the cleaner changes that I proposed in the text earlier, we're still going to need access to this print() method. So I'm going to revert these [noscript] annotations as well. Unfortunately that means a full rebuild is in order before I'll be able to test them, which will take until the morning.

So I've set the build going. Let's see what this looks like tomorrow.

If you'd like to catch up one of the other 99 diary entries, they're all available on my Gecko-dev Diary page.