1638913 - Refactor the download PDF preview to remove non-toolkit components and browser assumptions

Status: NEW

(Firefox :: Downloads Panel, defect, P3)

Description

[Sam Foster [:sfoster] (he/him)]

Bug 1191591 added changes to open downloaded PDFs in a new tab in a browser window. This requires both the logic in toolkit's DownloadIntegration.launchDownload method which negotiates mime handling and user preferences, as well as identifying and sometimes opening a window to open the file: URI in.

In practice, non-browser consumers of the DownloadIntegration will only resolve the lazy getter to the browser/ modules if they call loadFileIn, but it might be possible to refactor to move these dependencies and browser-window assumptions out to browser/components/Download/DownloadsCommon, if DownloadIntegration gets a way of informing callers what the outcome of launching a download will be.

Comment 1

[:Paolo Amadini]

DownloadIntegration is accessed via "Integration.jsm", which should already allow the applications to define lazy overrides for some of its functions, see for example the documentation here:

https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/browser/modules/PermissionUI.jsm#9-59
https://searchfox.org/mozilla-central/rev/9193635dca8cfdcb68f114306194ffc860456044/toolkit/modules/Integration.jsm#27-38

If some part of the process of opening a downloaded file has to be browser-specific and there is a meaningful, safe alternative for non-browser applications, you might be able to move it to a separate function and it might be possible to define a default non-browser implementation, as well as a browser override for it.

I don't know where the best place for calling "Integration.downloads.register" would be, but as long as it's guaranteed to run only once and before any download is started or resumed, that should work fine. That call won't load any of the download modules until they're needed later.

Comment 2

[[ex-Mozilla] Agi Sferro | :agi]

I don't believe GeckoView uses any of the code defined in toolkit/components/downloads, we define our own (very simple) nsIExternalHelperAppService which I believe is we don't need to define stuff like nsITransfer.

I removed the toolkit download component from the build and the try came back green IIRC, we could have untested paths however (which would be nice to find).

As far as GeckoView is concerned anything that can be moved to /browser can be.