426742 - Use correct filename in "Save Link As..." (SeaMonkey)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Robert Kaiser]

Bug 299372 has now fixed Firefox to respect Content-Disposition filenames in "Save Link As...", we should make the same strategy work in SeaMonkey.
This might depend on bug 381157, but I'm not 100% sure currently (looks like they have some part of the patch in the helper app dialog though).

Comment 1

[[e-mail]]

Actually I think they patched all versions of that dialog, didn't they?

Comment 2

[Robert Kaiser]

They only added the parameter to the function for other dialogs than the toolkit one, but not any handling of that parameter, so we need to make the xpfe dialog handle the param if we'd implement this without the dlmgr change in bug 381157.
Most of the code for this feature is in the context menu file though, from what I see in attachment 313264 [details] [diff] [review] (nsContextMenu.js in browser/).

Comment 3

[Philip Chee]

> They only added the parameter to the function for other dialogs than the
> toolkit one, but not any handling of that parameter

Yes they did add handling:
<http://mxr.mozilla.org/comm-central/source/mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js#145>
-        if (autoDownload && dir && dir.exists()) {
+        if (!aForcePrompt && autoDownload && dir && dir.exists()) {

By the way Firefox packages a different version of nsHelperAppDlg.js in dist/bin/components/
<http://mxr.mozilla.org/comm-central/find?text=&kind=text&string=nsHelperAppDlg.js>
That one has a dependency on the download manager in /mozapps/. When we do switch to the new download manager backend, we might want to package that version too.

Comment 4

[Matthias Versen [:Matti]]

There is one major difference between Firefox and SM with downloads the we should never lose : If you get a download from a content-disposition:attachment or if Gecko did content sniffing then SM tells you that in the download dialog, in Firefox you don't know why you get a download dialog which is very bad.
Ignore my comment if this bug doesn't modify this

Comment 5

[Robert Kaiser]

(In reply to comment #4)
> If you get a download from a content-disposition:attachment
> or if Gecko did content sniffing then SM tells you that in the download dialog,
> in Firefox you don't know why you get a download dialog which is very bad.

That's a different bug that SeaMonkey has now as well because bug 381157 landed. please file an own followup on that, we may be able to solve that with an overlay (if someone finds time and motivation to do that).


For the bug here, we should be able to relatively straight-forward port bug 299372 now, is someone CCed on here who can do this?

Comment 6

[Jens Hatlak (:InvisibleSmiley)]

Attachment: rough implementation

(In reply to comment #5)
> For the bug here, we should be able to relatively straight-forward port bug
> 299372 now, is someone CCed on here who can do this?

Yes (for both). :-)

Comment 7

[[e-mail]]

Comment on attachment 380496 [details] [diff] [review]
rough implementation

>+        var doc =  this.target.ownerDocument;
Nit: two spaces after =

>+        urlSecurityCheck(this.linkURL(), doc.nodePrincipal);
Hmm... possibly should be this.target.nodePrincipal

>+        function saveAsListener() {}
>+        saveAsListener.prototype = {
Bah, I had a look at the original bug, and it looks like biesi wanted this change, whereas I'd have preferred a simple object, although I guess that's going to be too hard at this late stage...

>+          getInterface: function sLA_callbacks_getInterface(aIID) {
I don't like these weird function names...

>+        function timerCallback() {}
>+        timerCallback.prototype = {
>+          notify: function sLA_timer_notify(aTimer) {
>+            channel.cancel(NS_ERROR_SAVE_LINK_AS_TIMEOUT);
>+            return;
>+          }
>+        }
At least here I think timer callbacks can be simple functions i.e.
function timerCallback() {
  channel.cancel(NS_ERROR_SAVE_LINK_AS_TIMEOUT);
}
timer.initWithCallback(timerCallback, timeToWait, timer.TYPE_ONE_SHOT);

>+        var channel = ioService.newChannelFromURI(this.getLinkURI());
Just use newChannel (see nsIIOService.idl)

>+        channel.notificationCallbacks = new callbacks();
That looks odd. But I see I made the same mistake in nsTypeAheadFind.js
(new findTypeController in my case).

>+        var timer = Components.classes["@mozilla.org/timer;1"]
>+                              .createInstance(Components.interfaces.nsITimer);
>+        timer.initWithCallback(new timerCallback(), timeToWait,
>+                               timer.TYPE_ONE_SHOT);
Perhaps we can use
var timer = setTimeout(timerCallback, timeToWait);
(and clearTimeout(timer) instead of timer.cancel() of course)?

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

(In reply to comment #7)
> >+        var channel = ioService.newChannelFromURI(this.getLinkURI());
> Just use newChannel (see nsIIOService.idl)

I looked it up. Other uses of that function seem to imply the second and third parameter should be null so that's what I did. I also stopped adding the getLinkURI() implementation since this was the only user.

> >+        channel.notificationCallbacks = new callbacks();
> That looks odd. But I see I made the same mistake in nsTypeAheadFind.js
> (new findTypeController in my case).

Which mistake? I don't understand. Is 'new' superfluous here or what are you trying to tell me? :-/

Comment 9

[[e-mail]]

(In reply to comment #8)
> I looked it up. Other uses of that function seem to imply the second and third
> parameter should be null so that's what I did. I also stopped adding the
> getLinkURI() implementation since this was the only user.
Well, the IDL I pointed you to did say that it's a shortcut for newChannelFromURI(newURI(..., ..., ...)) thus clarifying the argument usage (i.e. copy what getLinkURI used to do for newURI).

> > >+        channel.notificationCallbacks = new callbacks();
> > That looks odd. But I see I made the same mistake in nsTypeAheadFind.js
> > (new findTypeController in my case).
> Which mistake? I don't understand. Is 'new' superfluous here or what are you
> trying to tell me? :-/
Lower case class name. (Excluding "ns"/"moz" etc. prefix on some classes.)

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #9)
> (In reply to comment #8)
> > I looked it up. Other uses of that function seem to imply the second and third
> > parameter should be null so that's what I did. I also stopped adding the
> > getLinkURI() implementation since this was the only user.
> Well, the IDL I pointed you to did say that it's a shortcut for
> newChannelFromURI(newURI(..., ..., ...)) thus clarifying the argument usage
> (i.e. copy what getLinkURI used to do for newURI).

In that case it fits. The only difference is that there is no try/catch around the call anymore now. Do we need it here?

> > > >+        channel.notificationCallbacks = new callbacks();
> > > That looks odd. But I see I made the same mistake in nsTypeAheadFind.js
> > > (new findTypeController in my case).
> > Which mistake? I don't understand. Is 'new' superfluous here or what are you
> > trying to tell me? :-/
> Lower case class name. (Excluding "ns"/"moz" etc. prefix on some classes.)

I changed callbacks->Callbacks and saveAsListener->SaveAsListener (same case I guess) locally. Anything else needed for r/sr=you?

Comment 11

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v3 [Checkin: comment 12]

As discussed via IRC: removed prefixes from functions introduced by the patch
and fixed all 15 whitespace errors.

Comment 12

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/e3d3494fa075

I'll leave it to Jens to mark this bug fixed. :)

Comment 13

[Jens Hatlak (:InvisibleSmiley)]

Almost four years... Finally. :-)

Comment 14

[Robert Roessler]

Well, maybe not so fast... ;)

This appears to have caused Bug 498152, which is about a new Save As dialog that started appearing on Save Link Target operations.

Comment 16

[:Hb]

Reopen, because targets which URLs have parameters create only an item in the DL Manager but fail to download.

Comment 17

[Philip Chee]

Please don't reopen old fixed bugs. Instead file a new bug and make it block this one. Thanks.