997723 - Regression: Unable to "Save as PDF"

Status: RESOLVED WORKSFORME

(Firefox for Android :: General, defect)

Description

[Teodora Vermesan (:TeoVermesan)]

Build: Firefox for Android 31.0a1 (2014-04-17)
Steps to reproduce:
1. Open a website
2. Go to Page -> Save as PDF

Expected results: The PDF file should be downloaded.
Actual results: There is no action.

Note: The yesterday's build (2014-04-16) is not affected

Comment 1

[u421692]

http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=025a433dd5eb&tochange=d49f986956ab

Bug 901360 has introduced this regression.

Comment 2

[:Margaret Leibovic]

James, do you have time to help us investigate this?

Comment 3

[James Gilbertson]

(In reply to :Margaret Leibovic from comment #2)
> James, do you have time to help us investigate this?

I'll have time after today.

Comment 4

[James Gilbertson]

Attachment: WIP - Convert BrowserApp.saveAsPDF from nsIDownload

BrowserApp.saveAsPDF was using a nsIDownload as it's progress listener. This patch converts it to using a custom nsIWebProgressListener, modeled on DownloadNotifications.jsm.

The notification is a bit wonky right now (at least for me in the emulator), but the PDF is saved.

This leaves me with the question of how to actually access the PDF once it's been saved. I suspect that the download manager was being used for that purpose, since otherwise I'm not sure why it was involved. If that's case, I can add code to register the PDF once it has been saved.

Comment 5

[Wesley Johnston (:wesj)]

Comment on attachment 8410113 [details] [diff] [review]
WIP - Convert BrowserApp.saveAsPDF from nsIDownload

Review of attachment 8410113 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/chrome/content/browser.js
@@ +1082,5 @@
> +    Downloads.getSystemDownloadsDirectory()
> +             .then(d => OS.Path.join(d, fileName))
> +             .then(p => OS.File.openUnique(p, { humanReadable: true }))
> +             .then(r => {
> +               printSettings.toFileName = r.path;

Haven't had a chance to test, but it doesn't look like this creates an entry in the DownloadsManager. I think we want that so that there's a way to get back to this file.

::: mobile/android/modules/PrintNotifications.jsm
@@ +20,5 @@
> +const strings = Services.strings.createBundle("chrome://browser/locale/browser.properties");
> +
> +const CANCEL_BUTTON = {
> +  buttonId: "cancel",
> +  // TODO: localize?

I'm fine reusing this string. Its unlikely users would have much time to cancel anyway...

Comment 6

[Wesley Johnston (:wesj)]

Since these bugs are on Aurora, we need to move to get them fixed soon. I played with this a bit today, but didn't find any way to add things to the download lists that weren't real downloads. i.e. You can't do things like write a custom Saver, or just add a Download object to the list. Paolo, can you provide any guidence on how you'd like this fixed?

Comment 7

[:Paolo Amadini]

(In reply to Wesley Johnston (:wesj) from comment #6)
> Since these bugs are on Aurora, we need to move to get them fixed soon. I
> played with this a bit today, but didn't find any way to add things to the
> download lists that weren't real downloads. i.e. You can't do things like
> write a custom Saver, or just add a Download object to the list. Paolo, can
> you provide any guidence on how you'd like this fixed?

The current approach would be to create an instance of nsITransfer from "@mozilla.org/transfer;1", similarly to what we do when saving web pages:

http://mxr.mozilla.org/mozilla-central/source/toolkit/content/contentAreaUtils.js#386

This creates a new Download object that uses DownloadLegacySaver. nsITransfer inherits from nsIWebProgressListener, though I'm not sure whether you can pass it directly to the "print" method or you need to provide an intermediate object to process the notifications.

Comment 8

[Teodora Vermesan (:TeoVermesan)]

"Save as PDF" works for me on latest Nightly (05/19) on LG Nexus 4 (Android 4.4.2)

Comment 9

[Aaron Train [:aaronmt]]

This works for me on Aurora (31).

Comment 10

[:Margaret Leibovic]

This should be fixed with the patches that landed in bug 901360.

Aaron, can you verify the new patches in bug 901360 didn't cause a regression here? If things are looking good, we can just close this out.