1483590 - Add optional 'fileName' property to the 'pageSettings' passed into tabs.saveAsPDF()

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P5)

Description

[dw-dev]

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

Steps to reproduce:

I am the developer of the tabs.saveAsPDF() API and of the Print Edit WE add-on that uses this API.

Feedback from users of Print Edit WE indicates they would like to be able to add a prefix and/or suffix (e.g domain or date or time) to the saved PDF file name. This feature would be implemented in Print Edit WE in a similar way to how the same feature has been implemented in Save Page WE.

Currently, tabs.saveAsPDF() defaults the saved file name to the page content title.

The proposal is to add an optional 'fileName' property to the pageSettings passed into tabs.saveAsPDF().  If the 'fileName' property is omitted, the saved file name would still default to the page content title.

I would like approval to go ahead and implement this change.

Comment 1

[dw-dev]

Shane,

Are you okay for me to go ahead with this small change, which will make the tabs.saveAsPDF() API more flexible?

Comment 2

[Shane Caraveo (:mixedpuppy)]

I'm fine with this.  I haven't looked at the api in a long time, so I don't recall how the save mechanism works.  I'd want tests to show that this does not allow the extension to get access to save to arbitrary locations on the filesystem, we'd probably want to find an additional reviewer who has a good grasp of potential abuses there.  

mconca for final ok.

Comment 3

[dw-dev]

Shane,

I don't believe there any security issues with this change.

When tabs.saveAsPDF() is initiated, the user is presented with a file picker (Save As dialog), which always opens in the folder where the last file was saved.  At present, the file name in the file picker defaults to the page content title.

The current code is as follows:

>   picker.init(activeTab.ownerGlobal, title, Ci.nsIFilePicker.modeSave);
>   picker.appendFilter("PDF", "*.pdf");
>   picker.defaultExtension = "pdf";
>   picker.defaultString = activeTab.linkedBrowser.contentTitle + ".pdf";

The only change I want to make is to set the picker.defaultString from the new pageSettings.toFileName property.  If the pageSettings.toFileName property is not present, the file name in the file picker would still default to the page content title.

The new code would be as follows:

>   picker.init(activeTab.ownerGlobal, title, Ci.nsIFilePicker.modeSave);
>   picker.appendFilter("PDF", "*.pdf");
>   picker.defaultExtension = "pdf";
>   if (pageSettings.toFileName !== null) {
>      picker.defaultString = pageSettings.toFileName;
>   } else {
>      picker.defaultString = activeTab.linkedBrowser.contentTitle + ".pdf";
>   }

This change is trivial and I don't think it should require any changes to the existing tests or require any special review.

Or am I missing something?

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to dw-dev from comment #3)
> Shane,
> 
> I don't believe there any security issues with this change.
> 
> When tabs.saveAsPDF() is initiated, the user is presented with a file picker
> (Save As dialog), which always opens in the folder where the last file was
> saved.  At present, the file name in the file picker defaults to the page
> content title.

I see, this is just setting the default name in the filepicker.  Assuming that works like the save-as dialog, it should be perfectly fine.

> The new code would be as follows:
> 
> >   picker.init(activeTab.ownerGlobal, title, Ci.nsIFilePicker.modeSave);
> >   picker.appendFilter("PDF", "*.pdf");
> >   picker.defaultExtension = "pdf";
> >   if (pageSettings.toFileName !== null) {
> >      picker.defaultString = pageSettings.toFileName;
> >   } else {
> >      picker.defaultString = activeTab.linkedBrowser.contentTitle + ".pdf";
> >   }
> 
> This change is trivial and I don't think it should require any changes to
> the existing tests or require any special review.

Yeah, it should be fine, would still be good to add a simple test.

Comment 5

[Mike Conca [:mconca]]

This enhancement is approved.

Comment 6

[jscher2000]

** Scope Creep Alert **

Pages that do not have a title show the proposed file name:

.pdf

So it might be useful to address that at the same time by using the page name if the title is blank. (Similar to the logic of Save Page As.)

Or maybe it should be a separate bug?

Comment 7

[dw-dev]

I think it is a good idea to address this issue as part of this bug fix.

If the new 'toFileName' is present, the saved file named would always be the 'toFileName'.

If the new 'toFileName' is not present, the saved filename would be the page title, or if the page does not have a title then the last segment of the page URL path.

Comment 8

[dw-dev]

I've finally found time to look at this enhancement and I now have a patch that works in my environment.

There is a new optional "toFileName" property in the PageSettings parameter object.

The saved file name (before the ".pdf" extension) is determined as follows, in decreasing priority order:

• the toFileName property value, if defined and not an empty string,
• the content title, if not an empty string
• the last segment of the page URL path (excluding any file type extension), if not an empty string
• otherwise "mozilla" (the default in the print-to-PDF core code when the printSettings.toFileName is an empty string)

The new code looks like:

      if (pageSettings.toFileName !== null && pageSettings.toFileName != "") {
        picker.defaultString = pageSettings.toFileName + ".pdf";
      }
      else if (activeTab.linkedBrowser.contentTitle != "") {
        picker.defaultString = activeTab.linkedBrowser.contentTitle + ".pdf";
      }
      else
      {
        let url = new URL(activeTab.linkedBrowser.currentURI.spec);
        let path = decodeURIComponent(url.pathname);

        if (path.substr(-1) == "/")
        {
          path = path.substr(0,path.length-1);
        }
        let i = path.lastIndexOf("/");
        if (i >= 0) {
          path = path.substr(i+1);
        }
        i = path.lastIndexOf(".")
        if (i >= 0) path = path.substr(0,i);

        if (path != "") {
          picker.defaultString = path + ".pdf";
        }
        else {
          picker.defaultString = "mozilla.pdf";
        }
      }

I will submit a patch for review shortly.

Comment 9

[dw-dev]

Jim, please can you make me the assignee for this bug.

Comment 10

[dw-dev]

Attachment: Bug 1483590 support passing a filename to saveAsPDF

Add optional 'toFileName' property to the 'pageSettings' object
passed into tabs.saveAsPDF().

Comment 11

[dw-dev]

Shane,

I have created the patch to add an optional 'toFileName' property to the 'pageSettings' object passed into tabs.saveAsPDF().

I have updated the schema (tabs.json), updated the saveAsPDF code (ext-tabs.js), and updated the automated test code (browser_ext_tabs_saveAsPDF.js) to test that the saved filename is correct when the 'toFileName' is a defined string and when it is undefined (null).

All 3 of these files passed the checks in eslint. I have done extensive informal testing with an updated version of my Print Edit WE extension. All of the automated tests are passed successfully.

Please can you review this patch.

Comment 12

[dw-dev]

Shane,

I have tried:

filename = Services.io.newURI(filename).QueryInterface(Ci.nsIURL).fileName;

but that throws an exception.

I have tried:

filename = Services.io.newURI("file:///" + filename).QueryInterface(Ci.nsIURL).fileName;

and that works, but does not give the required result.

For example, if filename contains "Wikipedia, the free encyclopedia", then the result is "Wikipedia,%20the%20free%20encyclopedia". This is fine for the last segment of a URL path, but not for a computer file name, where "Wikipedia, the free encyclopedia" is perfectly valid and would be expected by a user.

Adding a "filename" FORMAT will have the same problem if it uses Services.io.newURI. Also, I'm not sure that the 'context' parameter would give access to 'activeTab.linkedBrowser.contentTitle' which is necessary to replicate the default 'Save As' behaviour in Firefox and Chrome.

The regex in the current revision replaces a superset of the characters that are not accepted by the major operating system file systems (Windows, Linux and Mac OS X), although in this case only Windows and Linux are relevant.

For me the distinction between a computer file name and a segment of a URL path is important to the user experience.

Maybe I am missing something or maybe it can be achieved in a different way?

Comment 13

[dw-dev]

From a user perspective, I think what we want to do is replicate the logic used in Firefox's normal "Save Page As" menu command.

The relevant sources are:

getDefaultFileName() - https://searchfox.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#1165
validateFileName() - https://searchfox.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#1271
sanitize() - https://searchfox.org/mozilla-central/source/toolkit/components/downloads/DownloadPaths.jsm#71

getDefaultFileName() generates a default file name for the file picker. getDefaultFileName() calls validateFileName() to validate the file name. validateFileName() just calls sanitize(), with some additional processing for Android.

In the current revision:

lines 1313-1329 implement a simplified version of the logic in getDefaultFileName(), giving priority to the name in pageSettings.toFileName.
line 1330 implements a simplified version of the logic in sanitize(), without any additional processing for Android.

I think it would be an improvement to replace the regex in line 1330 with a call to validateFileName().

Please see the new revision with this change.

Comment 14

[Shane Caraveo (:mixedpuppy)]

(In reply to dw-dev from comment #13)

validateFileName() - https://searchfox.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#1271

Good find. The point of using a FORMAT is that we can simply have that handled via schema for any future need as well. But I would call it localFilename or something to indicate it's intended use. However, it's probably best to just validate all the potential inputs.

Comment 15

[Shane Caraveo (:mixedpuppy)]

@dw-dev thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/262718256e32
support passing a filename to saveAsPDF r=mixedpuppy

Comment 17

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/262718256e32

Comment 18

[Richard Bloor]

toFileName added to tabs.PageSettings and compatibility data is merged.

Please let me know if there need to be any changes.

Comment 19

[dw-dev]

Looks fine. Thanks.

Comment 20

[dw-dev]

Works in released Firefox 75.

Comment 21

[dw-dev]

Is any more information required?