Download annotations are not stored for files without a custom name

RESOLVED FIXED in mozilla11

Status

()

defect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Depends on 1 bug)

Trunk
mozilla11
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

The downloads/destinationFileURI and downloads/destinationFileName are not stored about files which didn't use a custom file name (directly clicking on the file and letting the download helper take action).

The destinationFileName wouldn't be necessary, but the destinationFileURI information is important and missing (we need that for the "Open containing folder" action.)
Assignee: nobody → felipc
Status: NEW → ASSIGNED

Comment 2

8 years ago
(In reply to Felipe Gomes (:felipe) from comment #1)
> target is null at AddDownload time for files with no custom name here
> http://hg.mozilla.org/mozilla-central/annotate/cd90cf2bad07/uriloader/
> exthandler/nsExternalHelperAppService.cpp#l1688

Ah, I see, OnStartRequest is invoked before we show the action selection dialog,
allowing the download to start before we know the final target name. The issue
is that the same function also adds the history entry for the download. Thanks
for tracking this down!

I suppose the simplest option would be to defer the creation of the history entry
until mFinalFileDestination is populated (in LaunchWithApplication or SaveToDisk).
This would mean that no history entry would be created for downloads that are
canceled before the final action and target path are chosen. Those downloads
wouldn't appear in the Download Manager or downloads panel in any case, so I
think this behavior can be even better for what we want to do. Thoughts?

Felipe, are you already working on a patch?
Yeah I am, I have moved the history entry to SaveToDisk. I'm trying to verify now if the no-mimetype case is working correctly: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/nsDownloadManager.cpp#2397
but I haven't been able to exercise that code path (it looks like my local nginx is forcing a mimetype on files, or it could be the mimetype sniffing code, i'm not sure yet)
Posted patch Patch (obsolete) — Splinter Review
Patch that moves the AddDownload timing from OnStartRequest to SaveToDisk, at which point the file name has already been decided.

The test was somewhat tricky because I couldn't use the typical manual addDownload() that other DM tests do because this test is about verifying that it's correctly called by the helper service. I based it off the test_unknownContentType_dialog_layout.xul. It worked very well in the end and it fails (but doesn't timeout) without the fix applied.

I really don't know though what I can do about the setTimeout(.., 1200) there. It's there because the acceptDialog() function is disabled for 1 second and simply doesn't do anything before..
Attachment #574805 - Flags: review?(sdwilsh)
(In reply to Felipe Gomes (:felipe) from comment #4)
> I really don't know though what I can do about the setTimeout(.., 1200)
> there. It's there because the acceptDialog() function is disabled for 1
> second and simply doesn't do anything before..
Can you reach into the dialog and make change things by chance (like enable the button)?
Posted patch Patch v2Splinter Review
(In reply to Shawn Wilsher :sdwilsh from comment #5)
> (In reply to Felipe Gomes (:felipe) from comment #4)
> > I really don't know though what I can do about the setTimeout(.., 1200)
> > there. It's there because the acceptDialog() function is disabled for 1
> > second and simply doesn't do anything before..
> Can you reach into the dialog and make change things by chance (like enable
> the button)?

Done. I'm calling directly what acceptDialog() was supposed to call. I had tried it before and it was causing some weird errors in the download process, but placing it inside an executeSoon solved it.
Attachment #574805 - Attachment is obsolete: true
Attachment #574805 - Flags: review?(sdwilsh)
Attachment #574998 - Flags: review?(sdwilsh)

Comment 7

8 years ago
I think we should add a history entry for the LaunchWithApplication case also,
because currently we add a download entry to the panel in that case, allowing
the same file to be launched again (though the target is saved in a temporary
directory, and is likely to disappear later). This gives us a consistent behavior
between the panel and the Downloads History view.
Comment on attachment 574998 [details] [diff] [review]
Patch v2

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

r=sdwilsh with comments addressed

::: toolkit/mozapps/downloads/tests/chrome/test_destinationURI_annotation.xul
@@ +67,5 @@
> +
> +  onDownloadStateChange: function(aState, aDownload) {
> +    if (aDownload.state == Ci.nsIDownloadManager.DOWNLOAD_FINISHED) {
> +      is(aDownload.source.spec, DOWNLOAD_URI, "file was downloaded");
> +      aDownload.targetFile.remove(false);

It's possible this might throw on windows due to virus scanning running.  You might want to account for that in this test.

@@ +90,5 @@
> +
> +let windowObserver = {
> +  observe: function(aSubject, aTopic, aData) {
> +    if (aTopic != "domwindowopened")
> +      return;

global-nit: brace all ifs please
Attachment #574998 - Flags: review?(sdwilsh) → review+
(In reply to Paolo Amadini from comment #7)
> I think we should add a history entry for the LaunchWithApplication case
> also,
> because currently we add a download entry to the panel in that case, allowing
> the same file to be launched again (though the target is saved in a temporary
> directory, and is likely to disappear later). This gives us a consistent
> behavior
> between the panel and the Downloads History view.
This should be fixed too.  Somehow I missed this comment before doing my review.
Posted patch Patch v2Splinter Review
So instead of having one for SaveToDisk and one for LaunchWithApplication, what about having it at InitializeDownload? This seems to be the best place to add the download to history.

This patch does this and also fixes the other review comments. Re-requesting review since I've changed the call to InitializeDownload, and also made a small change to the test because it was leaking on MacOS: it was trying to close the dmui before it finished loading, so now I wait for the download-manager-ui-done notification before finishing the test.
Attachment #576073 - Flags: review?(sdwilsh)

Comment 11

8 years ago
(In reply to Felipe Gomes (:felipe) from comment #10)
> So instead of having one for SaveToDisk and one for LaunchWithApplication,
> what about having it at InitializeDownload? This seems to be the best place
> to add the download to history.

Comments in CreateProgressListener mention the case where NS_TRANSFER_CONTRACTID
is unavailable; I'm not sure whether we still want to create a history entry if
that's the case, but tentatively I'd say yes. This concerns third-party embedders,
more than Mozilla applications, I guess.
(In reply to Paolo Amadini from comment #11)
> Comments in CreateProgressListener mention the case where
> NS_TRANSFER_CONTRACTID
> is unavailable; I'm not sure whether we still want to create a history entry
> if
> that's the case, but tentatively I'd say yes. This concerns third-party
> embedders,
> more than Mozilla applications, I guess.
It should only be unavailable in embedding environments, so I'm not too worried.
Comment on attachment 576073 [details] [diff] [review]
Patch v2

per irc, carrying forward r+
Attachment #576073 - Flags: review?(sdwilsh) → review+
Target Milestone: --- → mozilla11
https://hg.mozilla.org/mozilla-central/rev/399ccb34b69b

if you wish you can avoid the [inbound] annotation in the sw, we don't use it.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Depends on: 572934
Depends on: 727961
Depends on: 779754
You need to log in before you can comment on or make changes to this bug.