Closed
Bug 701607
Opened 13 years ago
Closed 13 years ago
Download annotations are not stored for files without a custom name
Categories
(Toolkit :: Downloads API, defect)
Toolkit
Downloads API
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: Felipe, Assigned: Felipe)
References
Details
Attachments
(2 files, 1 obsolete file)
7.71 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
8.34 KB,
patch
|
Felipe
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•13 years ago
|
||
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
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Comment 2•13 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?
Assignee | ||
Comment 3•13 years ago
|
||
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)
Assignee | ||
Comment 4•13 years ago
|
||
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)
Comment 5•13 years ago
|
||
(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)?
Assignee | ||
Comment 6•13 years ago
|
||
(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•13 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 8•13 years ago
|
||
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+
Comment 9•13 years ago
|
||
(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.
Assignee | ||
Comment 10•13 years ago
|
||
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•13 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.
Comment 12•13 years ago
|
||
(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.
Assignee | ||
Comment 13•13 years ago
|
||
Comment on attachment 576073 [details] [diff] [review]
Patch v2
per irc, carrying forward r+
Attachment #576073 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 14•13 years ago
|
||
Whiteboard: [inbound]
Assignee | ||
Updated•13 years ago
|
Target Milestone: --- → mozilla11
Comment 15•13 years ago
|
||
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
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•