1415507 - Add progress listener and automated test for tabs.saveAsPDF()

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[dw-dev]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20171024165158

Steps to reproduce:

This is a follow-on to Bug 1404681 to add a progress listener and automated test for tabs.saveAsPDF().

The necessary patch was developed, tested and partially reviewed while investigating Bug 1404681.

Comment 1

[dw-dev]

Bob,

Comment 2

[dw-dev]

Bob,

I've been getting reports of the 'Save As PDF' feature in my Print Edit WE add-on not working correctly with Firefox 56 on Ubuntu and Fedora.

The PDF file is being saved, but it is in the wrong location.  It is being saved in the present working directory or home directory with the filename "mozilla.pdf", instead of the directory and filename specified by the user in the 'Save As' dialog.

I have done some investigation and this problem only happens if e10s is enabled. It appears that the print settings passed into tabs.saveAsPDF() are being re-initialized.  This results in 'printSettings.toFileName' being overwritten and my guess is that the other print settings are also being overwritten.  I suspect this problem is something to do with the change to avoid the printer name being accessed from the child process.

The code in tabs.saveAsPDF() looks like this:

saveAsPDF(pageSettings) {

  ....

  let psService = Cc["@mozilla.org/gfx/printsettings-service;1"].getService(Ci.nsIPrintSettingsService);
  let printSettings = psService.newPrintSettings;

  printSettings.printToFile = true;
  printSettings.toFileName = picker.file.path;

  printSettings.printSilent = true;
  printSettings.showPrintProgress = false;

  printSettings.printFrameType = Ci.nsIPrintSettings.kFramesAsIs;
  printSettings.outputFormat = Ci.nsIPrintSettings.kOutputFormatPDF;

  if (pageSettings.orientation !== null) {
    printSettings.orientation = pageSettings.orientation;
  }

  ....

  if (pageSettings.marginBottom !== null) {
     printSettings.marginBottom = pageSettings.marginBottom;
  }

  ....

  activeTab.linkedBrowser.print(activeTab.linkedBrowser.outerWindowID, printSettings, printProgressListener);

}

I have tried setting 'printSettings.isInitializedFromPrinter' and 'printSettings.isInitializedFromPrefs' to 'true' to avoid the re-initialization of the print settings, but that does not seem to work.

I think there needs to be an explicit way to avoid re-initialization of the print settings.  Also, there should be no dependency on 'printSettings.printerName'.

Please could you take a look to see what needs to be done?

Comment 3

[dw-dev]

Bob,

I've just had an idea ...

Would setting an arbitrary non-existent printer name as well as setting the initialization flags work?

For example:

  printSettings.printerName = "SAVEASPDF";
  printSettings.isInitializedFromPrinter = true;
  printSettings.isInitializedFromPrefs = true;

Comment 4

[Bob Owen (:bobowen)]

I'll try and have a look, it will probably be Monday at least before I can get to it.

Comment 5

[dw-dev]

Bob,

It would be great if could take a look.  Here is some more information ...

The basic point is that the print settings passed into FrameLoader.print() by tabs.saveAsPDF() are the settings that we want to use - and the settings should not be re-initialized by InitPrintSettingsFromPrinter or by InitPrintSettingsFromPrefs.

I have looked through the code in nsGlobalWindow.cpp and PrintingParent.cpp.

If in tabs.saveAsPDF() we set:

  printSettings.printerName = "";
  printSettings.isInitializedFromPrinter = true;
  printSettings.isInitializedFromPrefs = true;

then I think the existing code would be okay, except for line 157 in PrintingParent.cpp:

  settings->SetIsInitializedFromPrinter(false);

One possible solution would be to have another print setting (e.g. printSettings.doNotReinitialize), which could be used to prevent printSettings.isInitializedFromPrinter and printSettings.isInitializedFromPrefs from being set to false.

And line 157 could be made conditional on printSettings.doNotReinitialize.

Comment 6

[dw-dev]

Bob,

Have you had a chance to look at this?

I am keen to get the tabs.saveAsPDF() API working correctly for Linux users.

Apologies for chasing.

Comment 7

[Bob Owen (:bobowen)]

(In reply to dw-dev from comment #6)
> Bob,
> 
> Have you had a chance to look at this?
> 
> I am keen to get the tabs.saveAsPDF() API working correctly for Linux users.
> 
> Apologies for chasing.

No problem and sorry for the delay, I got a bit buried last week.
I can start looking at this now (I think I saw someone file a similar bug actually).

Comment 8

[Bob Owen (:bobowen)]

You were right this was because of the change to remove default printer name access from the child on Linux.
At [1] we always set an empty name to the default and that resets the isInitializedFromPrinter/Prefs.

I said in the review that I was concerned that might cause problems, sorry. :-(

I think if I change that to check for printToFile we should be OK, I'll file another bug.
I think that's the only legitimate reason for not having a printer name.

[1] https://hg.mozilla.org/mozilla-central/file/b6bed1b710c3/toolkit/components/printingui/ipc/PrintingParent.cpp#l146

Comment 9

[dw-dev]

Attachment: Bug 1415507 - changes to tabs.saveAsPDF();

There are two changes:

1. Adds a print progress listener to tabs.saveAsPDF() in ext-tabs.js so that the
  'saved' or 'replaced' status is not returned until the PDF file has been saved.

2. Adds an automated test for tabs.saveAsPDF() in browser_ext_tabs_saveAsPDF.js.

Review commit: https://reviewboard.mozilla.org/r/203904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203904/

Comment 10

[dw-dev]

Kris,

Commentary on patch:

1. This patch is substantially based on the initial patch I submitted for Bug 1404681.

2. This patch takes account of all of the comments you made on that initial patch in Comment 40 of Bug 1404681.

3. In Comment 44 of Bug 1404681, I explained why we need the 'locked file' workaround in tabs.saveAsPDF().

4. I have made all the changes you suggested in your review, except for changing 'resolve' to 'reject'.  When I first implemented tabs.saveAsPDF(), there was a lengthy discussion with Shane Caraveo about whether the 'not_saved' and 'not_replaced' cases should be handled as exceptions (reject) or just as returned statuses.  We agreed they should be handled as returned statuses, and this is how the interface is documented. Not being able to save a file for legitimate reasons, such as a file being locked or having restricted permissions, is not really an exception.  The calling add-on will be able to decide what action to take based on the returned status.

5. I have delayed submitting this patch until Bug 1420171 was fixed. When using Firefox e10s on Linux, that bug caused the PDF file to be written to the PWD or HOME directory instead of the directory specified in the Save As dialog.  Three more print settings are now initialized in tabs.saveAsPDF() to ensure that the other settings are not subsequently reinitialized by the print code:

    printSettings.printerName = "";
    printSettings.isInitializedFromPrinter = true;
    printSettings.isInitializedFromPrefs = true;

Comment 11

[dw-dev]

Shane,

This bug relates to the browser.tabs.saveAsPDF() interface, developed under Bug 1269300, for which you acted as my mentor.

The patch I have submitted here is very similar to the first patch I submitted for Bug 1404681.  However, the final patch for Bug 1404681 was stripped down to a simple one line change, because it had to be uplifted fo Firefox 57 beta.

The first patch I submitted for Bug 1404681 was reviewed by Kris, and his comments have been addresed, as described above in Comment 10.

I suspect that Kris is very busy at the moment.

Would you be able to review this patch?

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932859 [details]
Bug 1415507 - changes to tabs.saveAsPDF();

https://reviewboard.mozilla.org/r/203904/#review211166

Looks good, just a couple more tests.

::: browser/components/extensions/test/browser/browser_ext_tabs_saveAsPDF.js:39
(Diff revision 1)
> +
> +    background: async function() {
> +      let pageSettings = {};
> +
> +      let status = await browser.tabs.saveAsPDF(pageSettings);
> +      browser.test.assertEq("saved", status, "saveAsPDF status");

I'd like to see tests for not_saved, replaced and not_replaced.

Comment 13

[dw-dev]

Comment on attachment 8932859 [details]
Bug 1415507 - changes to tabs.saveAsPDF();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203904/diff/1-2/

Comment 14

[dw-dev]

Hi Shane,

Sorry it's taken so long to update the patch, but I have been working on updates to Print Edit WE and my other ad-ons.

The changes since the last patch are:

1. In tabs.json:

     - added four new "edge" properties in pageSettings to allow positioning of page headers and footers.
     - re-ordered the properties in pageSettings to be more logical.

2. In saveAsPDF() in ext-tabs.js: 

     - added initialization of the four new "edge" properties in pageSettings.
     - re-ordered the initialization of the properties in pageSettings to be consistent with tabs.json.

3. In browser_ext_tabs_saveAsPDF.js:

     - added four new tests for the other return statuses: replaced, canceled, not_saved and not_replaced.
     - all of these tests are passed successfully.

It was difficult to test the "not-saved" case, since this status will only be returned if the file does not already exists and the file cannot be saved.  The only way I could find to make the save fail was to create a directory with the same name as the file, which then prevents the file being saved.

Comment 15

[dw-dev]

Shane,

Unfortunately, it took me a bit of time to address the comment from your first review.

I submitted a revised patch a couple of weeks ago.

Please could you review this revised patch?

Comment 16

[dw-dev]

Shane,

Unfortunately, it took me a bit of time to address the comment from your first review.

I submitted a revised patch a couple of weeks ago.

Please could you review this revised patch?

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8932859 [details]
Bug 1415507 - changes to tabs.saveAsPDF();

https://reviewboard.mozilla.org/r/203904/#review219028

Can you do some cleanup on the test file?  Lots of repetetive code there that I think could be reduced.

::: browser/components/extensions/test/browser/browser_ext_tabs_saveAsPDF.js:6
(Diff revision 2)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +add_task(async function testSaveAsPDF_saved() {
> +  await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.net/");

let tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, "http://example.net/");

Then use the tab for the remove call later.

::: browser/components/extensions/test/browser/browser_ext_tabs_saveAsPDF.js:8
(Diff revision 2)
> +  let saveDir = FileUtils.getDir("TmpD", [`testSaveDir-${Math.random()}`], true);
> +
> +  let saveFile = saveDir.clone();
> +  saveFile.append("testSaveFile.pdf");
> +  if (saveFile.exists()) {
> +    saveFile.remove(false);
> +  }

lots of boiler plate here reused in all tests.  The tmpd could be in a setup/teardown for the entire test file.  saveFile could be a function called for each test.  Seems the filepicker could also be handled that way.  I wonder if there could be a test function that takes an options arg.

Comment 18

[dw-dev]

Comment on attachment 8932859 [details]
Bug 1415507 - changes to tabs.saveAsPDF();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203904/diff/2-3/

Comment 19

[dw-dev]

Shane,

I have submitted a revised patch taking account of your comments.

The only significant changes since the last patch are in browser_ext_tabs_saveAsPDF.js.

I had thought previously about creating a single test function, but couldn't quite see how to do it. I looked through the test files for the other API's and this helped me factorize the tests into a single function. The only tricky thing was how to pass the expected status into the extension, which I do through the manifest description field. The file has been reduced from 264 lines to 104 lines.

All of the tests are passed.

Comment 20

[Shane Caraveo (:mixedpuppy)]

nice.  r+ again.  and thanks for the test additions, that's great.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9687e3d987be
changes to tabs.saveAsPDF(); r=mixedpuppy

Comment 22

[Noemi Erli[:noemi_erli]]

Backed out changeset 9687e3d987be (bug 1415507) for bc failures in browser/components/extensions/test/browser/browser_ext_tabs_saveAsPDF.js on a CLOSED TREE

Backed out push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=9687e3d987bea9abf622c6e67ea10c77afa15391&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running

Log failure: https://treeherder.mozilla.org/logviewer.html#?job_id=157361440&repo=autoland&lineNumber=2992


Backout: https://hg.mozilla.org/integration/autoland/rev/f7f3280ea84a5933e3a8484d73ee600f04396e9b

:andym Could you please take a look?

Comment 23

[dw-dev]

This failure is happening because the tabs.saveAsPDF() API is not supported on Mac OS X and returns a rejected promise with the message "Not supported on Mac OS X".

Is there a way to not run this test for Mac OS X ?

Or should the test file (browser_ext_tabs_saveAsPDF.js) on Mac OS X catch this error, check the error message and notify the test as passed?

Comment 24

[Bob Owen (:bobowen)]

You need a line something like [1] in the ini file.
If it's windows only then:
skip-if = os != 'win' # <suitable comment>

[1] https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/browser/components/extensions/test/browser/browser-common.ini#198

Comment 25

[dw-dev]

Shane,

I have created a revised patch so that the test file is not run on Mac OS X, but I cannot submit this revised patch until the request is reopened.

Please can you reopen the review request.

Comment 26

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Does the "reopen" button at https://reviewboard.mozilla.org/r/203902/ not work for you? As far I know, it's only possible for the patch authors to reopen issues.

Comment 27

[dw-dev]

Comment on attachment 8932859 [details]
Bug 1415507 - changes to tabs.saveAsPDF();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203904/diff/3-4/

Comment 28

[dw-dev]

Shane,

I have submitted a revised patch that avoids running the test file on Mac OS X.

The only change since the last patch is adding one line in browser-common.ini as suggested by Bob.

I have assumed that this test file will not be run on Android?

Comment 29

[Shane Caraveo (:mixedpuppy)]

(In reply to dw-dev from comment #28)
> Shane,
> 
> I have submitted a revised patch that avoids running the test file on Mac OS
> X.
> 
> The only change since the last patch is adding one line in
> browser-common.ini as suggested by Bob.
> 
> I have assumed that this test file will not be run on Android?

You should run a try that includes android.  If everything runs clean, then resubmit checkin-needed.

Comment 30

[dw-dev]

Shane,

There are a few things I don't understand:

1. On this MDN page, it says that tabs.saveAsPDF is NOT supported on Firefox for Android:
   https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Browser_support_for_JavaScript_APIs

2. In the browser-common.ini file, there are no "skip-if = os == 'android'" statements for other APIs (e.g. tabs.discard, tabs.move, tabs.getZoom/setZoom) that are not supported on Firefox for Android.  So I had assumed that none of these test files were run on Android.

3. After the previous patch was pushed, the automated test was reported as failing on Mac OS X (Comment 22). Does this imply that the automated test was successful on Windows and Linux?


If a push to the try server is required, please could you initiate this, since I don't have commit access.

Comment 31

[Shane Caraveo (:mixedpuppy)]

(In reply to dw-dev from comment #30)
> Shane,
> 
> There are a few things I don't understand:
> 
> 1. On this MDN page, it says that tabs.saveAsPDF is NOT supported on Firefox
> for Android:
>   
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> Browser_support_for_JavaScript_APIs

Is there a question for that point?

> 2. In the browser-common.ini file, there are no "skip-if = os == 'android'"

It's not needed since this is under browser.  If it were under toolkit it would be necessary to address any android issues.  The try I just started includes android anyway.

> 3. After the previous patch was pushed, the automated test was reported as
> failing on Mac OS X (Comment 22). Does this imply that the automated test
> was successful on Windows and Linux?

Yeah.

> 
> If a push to the try server is required, please could you initiate this,
> since I don't have commit access.

done.

Comment 32

[dw-dev]

> 1. On this MDN page, it says that tabs.saveAsPDF is NOT supported on Firefox for Android:
>   
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Browser_support_for_JavaScript_APIs

Sorry, I should have made my question explicit:

  - If tabs.saveasPDF is not supported on Firefox for Android, then why would we need (or want) to test it on Android?


And the corollary:

  - If the try run on Android succeeds, is the plan then to support tabs.saveAsPDF on Android?

Comment 33

[Shane Caraveo (:mixedpuppy)]

(In reply to dw-dev from comment #32)
> > 1. On this MDN page, it says that tabs.saveAsPDF is NOT supported on Firefox for Android:
> >   
> > https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Browser_support_for_JavaScript_APIs
> 
> Sorry, I should have made my question explicit:
> 
>   - If tabs.saveasPDF is not supported on Firefox for Android, then why
> would we need (or want) to test it on Android?

The tests in this patch wouldn't run there, but a double check on try that we didn't break something on android is occasionally useful.

> And the corollary:
> 
>   - If the try run on Android succeeds, is the plan then to support
> tabs.saveAsPDF on Android?

Tests under "browser/" shouldn't run on android, only if they are under "toolkit/".  I dont think we need a plan to support this on android right now, but I'm uncertain if it's possible to or not.  Would be good to ask someone who knows the android code, and if it's easy, make a bug for it.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/42e3f329c6b7
changes to tabs.saveAsPDF(); r=mixedpuppy

Comment 35

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/42e3f329c6b7

Comment 36

[dw-dev]

Shane,

Are you okay for me to uplift this to mozilla59 beta?

Comment 37

[Shane Caraveo (:mixedpuppy)]

I'm fine to request that.  The changes are limited to one specific api call and addresses an api failure.

Comment 38

[dw-dev]

Comment on attachment 8932859 [details]
Bug 1415507 - changes to tabs.saveAsPDF();

Approval Request Comment
[Feature/Bug causing the regression]: tabs.saveAsPDF returned status before PDF file was saved.
[User impact if declined]: Potential undetected save failures.
[Is this code covered by automated tests?]: Yes - Automated tests added in this bug fix.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Changes are limited to one specific api call.
[String changes made/needed]: None

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8932859 [details]
Bug 1415507 - changes to tabs.saveAsPDF();

Looks like useful followup work from 57, let's try it in 59 beta 6. 
Thanks for the new tests.

Comment 40

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8321f5cb428

Comment 41

[Will Bamberg [:wbamberg]]

I've updated https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/tabs/PageSettings with the edge*. Is that everything for this bug?

Comment 42

[dw-dev]

Yes, that is the only change to the API.