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

RESOLVED FIXED in seamonkey2.0b1

Status

SeaMonkey
General
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Robert Kaiser, Assigned: InvisibleSmiley)

Tracking

(Blocks: 1 bug, {regression})

Trunk
seamonkey2.0b1
regression
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
Actually I think they patched all versions of that dialog, didn't they?
Keywords: helpwanted
(Reporter)

Comment 2

9 years ago
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

9 years ago
> 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.
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
(Reporter)

Updated

9 years ago
Depends on: 381157
(Reporter)

Updated

8 years ago
Flags: wanted-seamonkey2?
(Reporter)

Comment 5

8 years ago
(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?
(Assignee)

Comment 6

8 years ago
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). :-)
Assignee: general → jh
Status: NEW → ASSIGNED
Attachment #380496 - Flags: review?(neil)

Comment 7

8 years ago
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)?
(Assignee)

Comment 8

8 years ago
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? :-/
Attachment #380496 - Attachment is obsolete: true
Attachment #380543 - Flags: review?(neil)
Attachment #380496 - Flags: review?(neil)

Comment 9

8 years ago
(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.)
(Assignee)

Comment 10

8 years ago
(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?
Keywords: helpwanted → regression
(Assignee)

Comment 11

8 years ago
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.
Attachment #380543 - Attachment is obsolete: true
Attachment #382060 - Flags: superreview?(neil)
Attachment #382060 - Flags: review?(neil)
Attachment #380543 - Flags: review?(neil)

Updated

8 years ago
Attachment #382060 - Flags: superreview?(neil)
Attachment #382060 - Flags: superreview+
Attachment #382060 - Flags: review?(neil)
Attachment #382060 - Flags: review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
(Reporter)

Comment 12

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

I'll leave it to Jens to mark this bug fixed. :)
Keywords: checkin-needed
Target Milestone: --- → seamonkey2.0b1
(Reporter)

Updated

8 years ago
Flags: wanted-seamonkey2?
(Assignee)

Updated

8 years ago
Attachment #382060 - Attachment description: patch v3 → patch v3 [Checkin: comment 12]
(Assignee)

Comment 13

8 years ago
Almost four years... Finally. :-)
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 14

8 years ago
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.
(Assignee)

Updated

8 years ago
Blocks: 498949
(Reporter)

Updated

8 years ago
Duplicate of this bug: 387894

Comment 16

8 years ago
Reopen, because targets which URLs have parameters create only an item in the DL Manager but fail to download.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

8 years ago
Please don't reopen old fixed bugs. Instead file a new bug and make it block this one. Thanks.
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.