Camino needs to be able to provide to the user a streamlined downloading experience, where the download is automatically saved into their system-specified download folder, and the download is auto-dispatched to the helper app when done (as controlled by a pref, off by default). We hacked nsExternalHelperAppService.cpp on the chimera branch to do this. We now need a cleaner way to do it on the trunk.
*struggles through the morass of downloading APIs* So you'd think that an embedder could just run a 'save to disk'-type download, and launch the file themselves at the end of it. But no! There's logic in nsExternalAppHandler::MoveFile that assumes that if the file is a 'save to disk' download, then it will already have a unique name so we don't need to uniquify the name. It assumes that if the file exists, the user had hit 'Replace' in the file picker, so if a similarly named file exists when done (which might be hours later), we can happily go ahead and replace it with our downloaded file. This is wrong. Here's what I think should happen. Implementors of 'PromptForSaveToFile' should, if the user hits 'Replace' in the file picker, promptly delete that file. Then, nsExternalAppHandler::MoveFile() should always uniquify the name, never replacing existing files. Of course, the real fix would be to remove the name salting altogether. Only then do users get what they really expect (specify a file location to save to, then way data being accumulated into that file).
not that camino cares, but don't forget that a user might want to use mozilla to resume a download into a file on disk (which mozilla didn't download).
So... is this a memo-to-self bug? Or rather, do we plan to try and fix this before fixing the general pre-downloading mess?
This is a "we have fix regressions in downloading in trunk-based camino builds". Patches coming.
Created attachment 117439 [details] [diff] [review] Fix nsExternalAppHandler::SaveToDisk when aNewFileLocation is non-null This change allows an embedder to call nsExternalAppHandler::SaveToDisk, providing a file to download to. It ensures that CreateProgressListener() and ProcessAnyRefreshTags() are called in this case.
Associated Camino changes are in the patch in bug 192461.
Comment on attachment 117439 [details] [diff] [review] Fix nsExternalAppHandler::SaveToDisk when aNewFileLocation is non-null this looks fine to me. Let me know whether you want an r or an sr.... And I'd really like to clean up these interfaces to make them actually useful to embeddors. Feedback is very welcome (someone else doing the changing is even more welcome.... ;) ). Interface elimination, creation, heavy modification, all are fair game here.
Comment on attachment 117439 [details] [diff] [review] Fix nsExternalAppHandler::SaveToDisk when aNewFileLocation is non-null I lxr'd, and no-one currently calls SaveToDisk with a non-null aNewFileLocation. So this won't break anything.
15 years ago
Attachment #117439 - Flags: superreview?(bzbarsky) → superreview+
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.