Last Comment Bug 426742 - Use correct filename in "Save Link As..." (SeaMonkey)
: Use correct filename in "Save Link As..." (SeaMonkey)
Status: RESOLVED FIXED
: regression
Product: SeaMonkey
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.0b1
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
:
Mentors:
: 387894 (view as bug list)
Depends on: 299372 381157
Blocks: 498949
  Show dependency treegraph
 
Reported: 2008-04-03 03:29 PDT by Robert Kaiser
Modified: 2009-11-11 07:23 PST (History)
18 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
rough implementation (9.45 KB, patch)
2009-05-29 12:14 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (7.90 KB, patch)
2009-05-29 16:21 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v3 [Checkin: comment 12] (7.76 KB, patch)
2009-06-07 14:26 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
neil: superreview+
Details | Diff | Splinter Review

Description Robert Kaiser 2008-04-03 03:29:31 PDT
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 neil@parkwaycc.co.uk 2008-04-09 01:32:01 PDT
Actually I think they patched all versions of that dialog, didn't they?
Comment 2 Robert Kaiser 2008-04-22 10:41:55 PDT
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 2008-07-27 10:11:05 PDT
> 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] 2008-07-28 01:41:15 PDT
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 2009-05-29 04:58:43 PDT
(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) 2009-05-29 12:14:25 PDT
Created attachment 380496 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-05-29 13:54:41 PDT
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) 2009-05-29 16:21:04 PDT
Created attachment 380543 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2009-05-29 16:44:03 PDT
(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) 2009-05-30 04:00:15 PDT
(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) 2009-06-07 14:26:10 PDT
Created attachment 382060 [details] [diff] [review]
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 2009-06-08 06:11:34 PDT
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) 2009-06-08 11:25:32 PDT
Almost four years... Finally. :-)
Comment 14 Robert Roessler 2009-06-13 13:24:33 PDT
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 15 Robert Kaiser 2009-08-06 18:09:04 PDT
*** Bug 387894 has been marked as a duplicate of this bug. ***
Comment 16 :Hb 2009-11-11 06:28:02 PST
Reopen, because targets which URLs have parameters create only an item in the DL Manager but fail to download.
Comment 17 Philip Chee 2009-11-11 07:23:58 PST
Please don't reopen old fixed bugs. Instead file a new bug and make it block this one. Thanks.

Note You need to log in before you can comment on or make changes to this bug.