569898 - Port |Bug 489994 - Downloads list should honor always remember helper application| to SeaMonkey

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, enhancement)

Description

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch

From bug 489994:

---------
preconditions:
1. msword installed and set in windows as the default .rtf handler
2. mswordviewer installed

steps:
1. load data:application/rtf,hi
2. when asked what to do, select browse and select Word Viewer
3. select always remember.
4. select ok
5. open downloads
6. double click the rtf item

expected results:
4, 6 - opens word viewer w/ the rtf document

actual results:
4 - opens word viewer
6 - opens word
---------

Note: Tested with Word and OOo Writer, same result.

Comment 1

[Robert Kaiser]

Comment on attachment 449027 [details] [diff] [review]
patch

>         // fake an nsIDownload with the properties needed by that function
>-        openDownload({displayName: selItemData[0].target,
>+        openDownload({id: selItemData[0].dlid,
>+                      displayName: selItemData[0].target,
>                       targetFile: getLocalFileFromNativePathOrUrl(selItemData[0].file)});

We're trying to fake an nsIDownload here, so it should behave like a real download item , we shouldn't later need to retrieve a download item for this from the called function.

Comment 2

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #1)
> We're trying to fake an nsIDownload here, so it should behave like a real
> download item , we shouldn't later need to retrieve a download item for this
> from the called function.

Well, then I'll have to define the MIMEInfo property right there, and since that information is not stored in selItemData[0] (which is a row from treeView.js, search for "attrs") I'll have to call getDownload() prior to that which again will get me an nsIDownload object which I could pass in directly instead of messing with those fake objects in the first place.

I think that those bogus/incomplete objects are calling for trouble, as you can see here. It's just a bad idea--if we're faking an object we should at least go the whole way and not just define what the called function *currently* needs (think future). And if certain assumptions are made, they should be documented at the definition of the called function, not (just) the call.

Another option would be doing it like FF for all such functions: just always pass in the whole selItemData[0] object and let the function take care of the rest. But I assume that option has already been rejected in the original DLMGR refresh bug...

Comment 3

[[e-mail]]

(In reply to comment #2)
> Another option would be doing it like FF for all such functions: just always
> pass in the whole selItemData[0] object and let the function take care of the
> rest. But I assume that option has already been rejected in the original DLMGR
> refresh bug...
The problem here is that the method has two callers, the progress dialog (which has the download object) and the download manager (which does not).

Comment 4

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #3)
> (In reply to comment #2)
> > Another option would be doing it like FF for all such functions: just always
> > pass in the whole selItemData[0] object and let the function take care of the
> > rest. But I assume that option has already been rejected in the original DLMGR
> > refresh bug...
> The problem here is that the method has two callers, the progress dialog (which
> has the download object) and the download manager (which does not).

So why don't we just get the actual download object using gDownloadManager.getDownload(selItemData[0].dlid) and pass that from cmd_open?

Comment 5

[[e-mail]]

OK, so this is the behaviour I see with the patch:
1. If the file was saved to disk, then opening the file from the download manager still always uses the default application.
2. If the file was opened with an application, then reopening the file from the download manager reuses the same application.
Note that you don't need to select always remember; that only affects subsequent downloads.

Comment 6

[[e-mail]]

Comment on attachment 449027 [details] [diff] [review]
patch

>   try {
>+    try {
>+      var download = gDownloadManager.getDownload(aDownload.id);
>+      var mimeInfo = download.MIMEInfo;
>+      if (mimeInfo.preferredAction == mimeInfo.useHelperApp) {
>+        mimeInfo.launchWithFile(file);
>+        return;
>+      }
>+    } catch (ex) { }
No point nesting this try/catch, just put it before.

(In reply to comment #4)
> So why don't we just get the actual download object using
> gDownloadManager.getDownload(selItemData[0].dlid) and pass that from cmd_open?
Seeing as it's hard to fake the mime info, this is probably the best we can do.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2

Notes:
1. used var and let depending on context (existing code in the vicinity)
2. added empty lines around try block in line with similar code above

Comment 8

[[e-mail]]

Comment on attachment 449363 [details] [diff] [review]
patch v2

>+    var mimeInfo = aDownload.MIMEInfo;
In theory this could be null. Would you mind null-checking it too please?

>+        let download = gDownloadManager.getDownload(selItemData[0].dlid);
>+        openDownload(download);
Nit: this could all be done on one line.

Comment 9

[Jens Hatlak (:InvisibleSmiley)]

Attachment: patch v2a, r=Neil [Checkin: comment 10]

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 449427 [details] [diff] [review]
patch v2a, r=Neil [Checkin: comment 10]

http://hg.mozilla.org/comm-central/rev/405bae780c96

Comment 11

[Robert Kaiser]

(In reply to comment #4)
> So why don't we just get the actual download object using
> gDownloadManager.getDownload(selItemData[0].dlid) and pass that from cmd_open?

This is surely an option, I just wanted to avoid getting that object where we have all data the called function needs, but in that case, we don't in the download tree array and so going for the actual download object seems OK.