1641805 - Stop calling nsIWebBrowserPrint.print() in DownloadCore.jsm

Status: RESOLVED FIXED

(Toolkit :: Downloads API, task, P3)

Description

[Jonathan Watt [:jwatt]]

For Fission, all printing will need to be initiated from the parent process. All code that calls nsIWebBrowserPrint.print in the child process needs to be rewritten to invoke printing via the parent process, or otherwise removed.

This bug covers the call here:

https://searchfox.org/mozilla-central/rev/9aa7bebfd169bc2ead00ef596498a406e56bbb85/toolkit/components/downloads/DownloadCore.jsm#2890

In this case Gijs pointed out that it looks like DownloadPDFSaver is something we probably had for fennec but likely no longer need, although there is some Sync serialization/deserialization code that uses DownloadPDFSaver, and some tests.

Lina, can you tell me if we sync download serializable objects in a way where we could still be constructing DownloadPDFSaver? (Per Gijs, if we do, we could presumably still remove the actual execute stuff that creates them, which is the bit that contains the print calls.)

Comment 1

[Lina Butler (ex-Mozilla)]

I've never heard of DownloadSaver til today! 😅 Would you mind pointing me to the Sync code that uses it? IIRC, we sync download history (by accident; bug 1469169 covers filtering it out because it's confusing), but syncing serialized download info objects—I guess that's what toSerializable and fromSerializable do—is definitely a no-no, and we should nix it!

Comment 2

[Jonathan Watt [:jwatt]]

Sorry, Lina, as a platform dev I'm not too familiar with the toolkit/frontend code. :) I was referring to DownloadSaver.fromSerializable, and I guess the assumption was that that is Sync code.

Digging deeper it seems that we get into DownloadPDFSaver.fromSerializable via the stack:

DownloadPDFSaver.fromSerializable
DownloadSaver.fromSerializable
Download.fromSerializable
Downloads.createDownload

Presumably this is not related to Sync. Also, Downloads.createDownload seems to be called by a bunch of code so it looks like this is also not unreachable/dead code.

Maybe DownloadPDFSaver is dead though, since to get to that, DownloadSaver.fromSerializable needs to be passed the string "pdf" and I can't see any code that contains "pdf" that may be a source of that.

Some of this code appears to originally come from the patch for bug 851454. Paolo, can you shed any light on whether DownloadPDFSaver can be removed?

Comment 3

[Jonathan Watt [:jwatt]]

Neil, since you reviewed that patch maybe you might also be able to provide some insight here?

Comment 4

[:Paolo Amadini]

DownloadPDFSaver was indeed added for Fennec in bug 1063217, and it can probably be removed, using the patch on that bug as a guide for what parts of the code have to be reverted.

The more elaborate answer is that the methods "toSerializable" and "fromSerializable" are involved in the creation of "downloads.json", which is stored in the profile folder to persist downloads across sessions. On Desktop, only downloads that need additional information to be resumed are serialized there, while the history of completed downloads is stored in the Places database. On Fennec, it used to be the case that completed downloads would be stored in "downloads.json" as well.

In the theoretical case, a download deserialized from "downloads.json" with the saver type "pdf" would trigger the creation of DownloadPDFSaver. However, since this object depends on a live nsIDOMWindow reference, any in-progress downloads of this type cannot be restarted, thus they are never serialized in the first place. Completed PDF downloads are serialized as if they started with the default "copy" saver:

https://searchfox.org/mozilla-central/rev/d9d492eda787a6eda66016e6f8398ee759f7bc25/toolkit/components/downloads/DownloadCore.jsm#2940

This means that, for all practical purposes, DownloadPDFSaver is not created anymore unless createDownload is invoked manually with the "pdf" saver type.

By the way, to the best of my knowledge, all the code in "DownloadCore.jsm" runs in the parent process, unless this was changed recently.

Comment 5

[Jonathan Kew [:jfkthame]]

So based on comment 4, my understanding is that we can simply remove DownloadPDFSaver and associated code & tests. I've pushed a patch that does this to tryserver to see if I missed anything critical, and will post it for review if all goes well.

Comment 6

[Jonathan Kew [:jfkthame]]

Not quite. Trying again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35a7a6d37e9da2ec1daf0e0b9a1c1f10bc736f27

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1641805 - Remove DownloadPDFSaver, no longer needed in a post-Fennec world. r=paolo,mak

Comment 8

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1641805 - Remove support for creating a DownloadSource from an nsIDOMWindow. r=mak

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8867d132ccba
Remove DownloadPDFSaver, no longer needed in a post-Fennec world. r=paolo,mak
https://hg.mozilla.org/integration/autoland/rev/39b68c2517c4
Remove support for creating a DownloadSource from an nsIDOMWindow. r=mak

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8867d132ccba
https://hg.mozilla.org/mozilla-central/rev/39b68c2517c4