Last Comment Bug 569898 - Port |Bug 489994 - Downloads list should honor always remember helper application| to SeaMonkey
: Port |Bug 489994 - Downloads list should honor always remember helper applica...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Download & File Handling (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: seamonkey2.1a2
Assigned To: Jens Hatlak (:InvisibleSmiley)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-03 09:15 PDT by Jens Hatlak (:InvisibleSmiley)
Modified: 2010-06-05 08:58 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (2.05 KB, patch)
2010-06-03 09:15 PDT, Jens Hatlak (:InvisibleSmiley)
no flags Details | Diff | Splinter Review
patch v2 (2.02 KB, patch)
2010-06-04 15:33 PDT, Jens Hatlak (:InvisibleSmiley)
neil: review+
Details | Diff | Splinter Review
patch v2a, r=Neil [Checkin: comment 10] (2.00 KB, patch)
2010-06-05 01:31 PDT, Jens Hatlak (:InvisibleSmiley)
jh: review+
Details | Diff | Splinter Review

Description Jens Hatlak (:InvisibleSmiley) 2010-06-03 09:15:57 PDT
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.
Comment 1 Robert Kaiser 2010-06-03 09:56:46 PDT
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) 2010-06-03 12:09:19 PDT
(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 neil@parkwaycc.co.uk 2010-06-04 08:25:08 PDT
(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) 2010-06-04 09:05:57 PDT
(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 neil@parkwaycc.co.uk 2010-06-04 09:10:55 PDT
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 neil@parkwaycc.co.uk 2010-06-04 09:17:21 PDT
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) 2010-06-04 15:33:22 PDT
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
Comment 8 neil@parkwaycc.co.uk 2010-06-04 16:11:56 PDT
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) 2010-06-05 01:31:55 PDT
Created attachment 449427 [details] [diff] [review]
patch v2a, r=Neil [Checkin: comment 10]
Comment 10 Jens Hatlak (:InvisibleSmiley) 2010-06-05 01:39:07 PDT
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 2010-06-05 08:58:55 PDT
(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.

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