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

RESOLVED FIXED in seamonkey2.1a2

Status

SeaMonkey
Download & File Handling
--
enhancement
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: InvisibleSmiley, Assigned: InvisibleSmiley)

Tracking

Trunk
seamonkey2.1a2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 449027 [details] [diff] [review]
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.
Attachment #449027 - Flags: review?(neil)

Comment 1

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

Comment 2

7 years ago
(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...
Summary: Port |Bug 489994 - Downloads list should honor always remember helper| to SeaMonkey → Port |Bug 489994 - Downloads list should honor always remember helper application| to SeaMonkey

Comment 3

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

Comment 4

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

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

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

Comment 7

7 years ago
Created attachment 449363 [details] [diff] [review]
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
Attachment #449027 - Attachment is obsolete: true
Attachment #449363 - Flags: review?(neil)
Attachment #449027 - Flags: review?(neil)

Comment 8

7 years ago
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.
Attachment #449363 - Flags: review?(neil) → review+
(Assignee)

Comment 9

7 years ago
Created attachment 449427 [details] [diff] [review]
patch v2a, r=Neil [Checkin: comment 10]
Attachment #449363 - Attachment is obsolete: true
Attachment #449427 - Flags: review+
(Assignee)

Comment 10

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

http://hg.mozilla.org/comm-central/rev/405bae780c96
Attachment #449427 - Attachment description: patch v2a, r=Neil → patch v2a, r=Neil [Checkin: comment 10]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.1a2

Comment 11

7 years ago
(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.
You need to log in before you can comment on or make changes to this bug.