Honour user preferences for "open file with [app]" when improvements_to_download_panel pref is set
Categories
(Firefox :: Downloads Panel, task, P3)
Tracking
()
People
(Reporter: Gijs, Assigned: bigiri)
References
Details
(Whiteboard: [fidefe-mr11-downloads])
Attachments
(1 file, 1 obsolete file)
This is basically bug 453455, but behind the pref, so I don't want us to mark that bug as fixed when in practice, users will still be seeing it until the improvements_to_download_panel pref change rides to release.
We should be able to change the code that does this at https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/uriloader/exthandler/nsExternalHelperAppService.cpp#1867-1877, if the pref is set. I think there's already an attempt there to do this, but I don't think it's 100% effective at the moment.
This is going to require some testing with different files (pdf, word doc, images), with/without the Content-Disposition: attachment
header, and various configurations for those file types to either save to disk or open with an external helper app, or internally, and verifying that we honour those prefs irrespective of the header.
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
Added unit tests to verify that pdf, word doc, and image files, with/without the Content-Disposition: attachment header, and various configurations for those file types to either save to disk or open with an external helper app, or internally are honored irrespective of the header.
Still incomplete:
- verify opening with helper app or saving to disk based on user preferences and not a simulated click
- bug in cleanup when running multiple tests
Assignee | ||
Comment 2•3 years ago
|
||
Please take a look at my WIP patch. My tests are throwing errors on cleanup but run fine if I only do one instead of looping through all cases. I'm not sure how to fix that.
Reporter | ||
Comment 3•3 years ago
|
||
It would have been helpful to include the error messages that are confusing you; right now I don't know if I'm seeing the same thing.
The error message on my machine is:
0:42.06 FAIL Cleanup function threw an exception - [Exception... "Unexpected error" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: resource://testing-common/httpd.js :: stop :: line 631" data: no]
The line number and filename point to this:
So something is calling stop()
, but the socket is already gone.
Then you look for what might be doing that, and having looked at your patch, I noticed this:
return Downloads.createDownload({
- source: httpUrl("interruptible.txt"),
+ source: httpUrl(file),
You're calling this helper that seems to have to do with the http server. So then I go check what that helper is: https://searchfox.org/mozilla-central/source/browser/components/downloads/test/browser/head.js#335
That's using a gHttpServer
, and it turns out all your testcases call startServer
- which overwrites that variable, and then adds a cleanup function. So then all the N cleanup functions for N tests call stop
on the same server, and things break.
startServer
isn't meant to be called more than once, because it's using that one global. Just once per test is sufficient. I'm not sure why you decided to call it in every testcase, or what test you copied that is using the server. I don't think you need that server for your testcase - I don't think you need your http server to be interruptible at all, and the mochitest framework already serves all your support-files on http://example.com/
, .org
, the https equivalent, and a pile of other hosts. Can you clarify why you think you need the interruptibility?
I would also have thought that you could start the download by clicking a link in content, and then waiting for the download object to be created, rather than creating download objects manually - the closer the test is to what happens to users, the better. Once you've got a download, you can check that we're set up to do the right thing with it and cancel it immediately. Does that sound like it'll work?
Assignee | ||
Comment 4•3 years ago
|
||
I've applied these changes. The only thing left is checking if the downloaded file is opened in Firefox, opened in an app, saved on disk, handled according to system defaults, or if the user was prompted to choose. Unfortunately, it doesn't appear that information is available from the JS environment.
Comment 5•3 years ago
•
|
||
(In reply to Bernard Igiri from comment #4)
I've applied these changes. The only thing left is checking if the downloaded file is opened in Firefox, opened in an app, saved on disk, handled according to system defaults, or if the user was prompted to choose. Unfortunately, it doesn't appear that information is available from the JS environment.
I think you can get this information off the mimeInfo
object, which can be retrieved from the nsIMIMEService. In particular, you can call getFromTypeAndExtension
. See https://searchfox.org/mozilla-central/source/uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js#148-153 for an example on this.
Assignee | ||
Comment 6•3 years ago
|
||
@Micah, (In reply to Micah [:mtigley] (she/her) from comment #5)
I'm using that to set the preferredAction
so I was hoping to find an independent way to verify that it's doing what that setting is asking for.
let mimeType = gHandlerService.getTypeFromExtension(fileExtension);
let mimeSettings = gMIMEService.getFromTypeAndExtension(
mimeType,
fileExtension
);
mimeSettings.preferredAction = preferredAction;
Reporter | ||
Comment 7•3 years ago
|
||
You should be able to determine whether the download manager will open the file using the launchWhenSucceeded
property on the download object, similar to this test: https://searchfox.org/mozilla-central/rev/d37daf2f82ed22b6a2a5cbbb975423825dfd69fa/browser/components/downloads/test/browser/browser_download_opens_on_click.js#74 .
You could also check that something calls launchDownload
, like the code at https://searchfox.org/mozilla-central/rev/d37daf2f82ed22b6a2a5cbbb975423825dfd69fa/toolkit/components/downloads/test/unit/common_test_Download.js#2366-2376 and elsewhere in that test.
For internally handled files, you should be able to check that a tab opens with the right file, and for saved-to-disk files, I'd expect verifying that the file gets saved and that launchWhenSucceeded
is false is sufficient (hard to otherwise verify that something doesn't happen, of course).
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
Apparently the Content-Disposition: attachment
header is necessary. It turns out that my testing method was only triggering the JavaScript and the native code that actually implemented the preferredAction
logic was not running. I have since changed my test modeling uriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js
to trigger the download by opening a new tab. Using this method I discovered that the header is necessary.
I also had trouble getting uriloader/exthandler/tests/mochitest/file_xml_attachment_binary_octet_stream.xml
to download using the alwaysAsk
preferred action. Although may be a fluke in my testing as alwaysAsk
had to be run separately from the other test conditions to get it to work. I suspect that something isn't being cleaned up properly. Regardless, the tests in the patch all complete successfully.
Should a new bug be filed to address the header issue?
Reporter | ||
Comment 9•3 years ago
|
||
(In reply to Bernard Igiri from comment #8)
Apparently the
Content-Disposition: attachment
header is necessary. It turns out that my testing method was only triggering the JavaScript and the native code that actually implemented thepreferredAction
logic was not running. I have since changed my test modelinguriloader/exthandler/tests/mochitest/browser_open_internal_choice_persistence.js
to trigger the download by opening a new tab. Using this method I discovered that the header is necessary.
I don't really understand this comment - the header is necessary... for what? What happens without the header?
I also had trouble getting
uriloader/exthandler/tests/mochitest/file_xml_attachment_binary_octet_stream.xml
to download using thealwaysAsk
preferred action. Although may be a fluke in my testing asalwaysAsk
had to be run separately from the other test conditions to get it to work. I suspect that something isn't being cleaned up properly. Regardless, the tests in the patch all complete successfully.
Again, I feel like there are details missing here - what exactly were you trying to do, what did you expect, and what happened instead? You've not described the failure case (you're implying the file didn't download - what happened instead? The patch says there were "errors" - what kind of errors?)
Should a new bug be filed to address the header issue?
I don't know what "the header issue" is. The point of this bug is primarily to ensure that, if a user has configured a filetype to "always open with [name of external application]" (either the system default or a separately configured external app), that that configuration is honoured for that filetype, both when it is served with a Content-Disposition: attachment
header, and when it isn't. It seemed to make sense to me in comment #0 to do the same testing for the other possible user configurations (always ask, save to disk, open internally). If the header is still influencing behaviour when the filetype is set to "open with [external app]", then the point of this bug is to address that. If you're running into issues with one of the other configurations (which one?) then that could probably be a separate bug.
Assignee | ||
Comment 10•3 years ago
•
|
||
(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:
- Attempting to download a png or webm file with
Content-Disposition: inline
- Attempting to download a text file without the
Content-Disposition
header at all
The test in the patch for this ticket makes it easy to add those kinds of test cases and more. That is why I want to separate this ticket from the work to resolve the behavior when the Content-Disposition
header is missing or set to a value other than attachment
.
Comment 11•3 years ago
|
||
(In reply to Bernard Igiri from comment #10)
(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:
- Attempting to download a png or webm file with
Content-Disposition: inline
- Attempting to download a text file without the
Content-Disposition
header at all
Can you explain what is meant by "failed to complete"? Does the download start and stay in the downloads panel permanently?
Assignee | ||
Comment 12•3 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
(In reply to Bernard Igiri from comment #10)
(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:
- Attempting to download a png or webm file with
Content-Disposition: inline
- Attempting to download a text file without the
Content-Disposition
header at allCan you explain what is meant by "failed to complete"? Does the download start and stay in the downloads panel permanently?
Yes.
Comment 13•3 years ago
•
|
||
(In reply to Bernard Igiri from comment #12)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #11)
(In reply to Bernard Igiri from comment #10)
(In reply to :Gijs (he/him) from comment #9)
File downloads have failed to complete under the following conditions:
- Attempting to download a png or webm file with
Content-Disposition: inline
- Attempting to download a text file without the
Content-Disposition
header at allCan you explain what is meant by "failed to complete"? Does the download start and stay in the downloads panel permanently?
Yes.
Can you describe more? Show a screenshot? Is there any message to the user? What is the state of the download object?
Assignee | ||
Comment 14•3 years ago
|
||
So it turns out that when I open a file that Firefox can display in a new tab with a Content-Disposition
other than attached
then Firefox will just open that file in the browser which is correct behavior. However as Gijs just pointed out to me, mime types that are not supported by Firefox should still be downloaded even without the Content-Disposition
header. So I added test cases with mime types application/ms-word
and application/x-7z-compressed
without the Content-Disposition
header and with Content-Disposition: inline
respectively, and the tests all pass. I even used a node server to simulate a host serving these files with those headers, and those also downloaded correctly.
I believe that this sufficiently shows that the browser is handling these mime types correctly.
Comment 15•3 years ago
|
||
(In reply to Bernard Igiri from comment #14)
Great, thanks!
Updated•3 years ago
|
Comment 16•3 years ago
|
||
The severity field is not set for this bug.
:mak, could you have a look please?
For more information, please visit auto_nag documentation.
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Comment 17•3 years ago
|
||
Attempting to use waitForNewTab with https://phabricator.services.mozilla.com/D127759 patch.
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 18•3 years ago
|
||
Comment 19•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Description
•