197745 - nsExternalAppHandler needs to be able to support auto-saving and auto-dispatching of downloaded files

Status: VERIFIED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Simon Fraser [no longer active]]

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.

Comment 1

[Simon Fraser [no longer active]]

*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).

Comment 2

[timeless]

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).

Comment 3

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Simon Fraser [no longer active]]

This is a "we have fix regressions in downloading in trunk-based camino builds".
Patches coming.

Comment 5

[Simon Fraser [no longer active]]

Attachment: 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.

Comment 6

[Simon Fraser [no longer active]]

Associated Camino changes are in the patch in bug 192461.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 8

[Simon Fraser [no longer active]]

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.

Comment 9

[Simon Fraser [no longer active]]

Checked in.

Comment 10

[Chris Petersen]

Verified