183848 - autoDispatch isn't working for some downloads (need to use Launch Services directly)

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Steve Dagley]

nsOSHelperAppService::LaunchAppWithTempFile is being called to dispatch the file
downloaded but the aMIMEInfo->GetPreferredApplicationHandler call on the
nsIMIMEInfo passed in doesn't retrieve an application.  This only happens for
some file types which leads me to believe we're running into a problem between
LaunchServices and InternetConfig again.  My proposed fix is to call
LSGetApplicationForItem() for the tempfile and see if launchServices does in
fact find a handler.

Comment 1

[Steve Dagley]

Attachment: Patch as described

Comment 2

[Steve Dagley]

Fixed

Comment 3

[Chris Petersen]

Excellent.. This now launches the default app for .pdf files and .rm files after
dl is completed. Tested with different default apps for .pdf (Acrobat and
Preview.app) and RealOne player (default app for .rm files). Added
user_pref("browser.download.autoDispatch", true); to my user.js. Tested with the
2002-12-06-04 NB under 10.2.2.

Comment 4

[Steve Dagley]

Reopening as this needs to be landed on the Mozilla trunk.

Comment 5

[Steve Dagley]

Comment on attachment 108446 [details] [diff] [review]
Patch as described

Adding r/sr requests for trunk landing

Comment 6

[Conrad Carlen (not reading bugmail)]

Comment on attachment 108446 [details] [diff] [review]
Patch as described

r=ccarlen

Comment 7

[Simon Fraser [no longer active]]

I have an alternative fix in bug 197745 that doesn't require #ifdefs in that
code (which is to do autoexanding from the frontend). I think it's cleaner.

Comment 8

[Steve Dagley]

Changing Product to Browser as this isn't a Camino speciifc change (and as Simon
commented he has a different fix for Camino that doesn't use 
nsOSHelperAppService::LaunchAppWithTempFile)

Comment 9

[Simon Fraser [no longer active]]

Clarify what the patch is about. Yes, we need this.

Comment 10

[saari (gone)]

->sfraser

Comment 11

[saari (gone)]

Simon, since bug 197745 is fixed, should this be duped to it and closed?

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 108446 [details] [diff] [review]
Patch as described

No, we still need this patch.

Comment 13

[Michael Buckland]

Discussed in edt bug triage.  Plussing.

Comment 14

[Simon Fraser [no longer active]]

Checked in.

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

this is interesting. this patch was merged to the trunk (at time of checkin)
rather wrongly.

This patch, as attached, has basically this (correct) logic:
if (application) {
 ...use it...
} else {
 ... ask launch services ...
}

As checked in, it does this, entirely different, thing:
if (aMimeInfo) {
 ... use the mimeinfo's application, return if none available ...
} else {
  ...this code is basically unreachable...
  ...were it reachable, it would talk to launchservices...
}


Presumably the following checkin is sorta responsible for this:
bug 86640, which changed the |if (application) { ... }| to |if (!application)
return some_error;|

my point is:
this code currently does NOTHING AT ALL. reopening bug.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Someone who has a mac build and can test needs to fix this up....

Comment 17

[Steve Dagley]

taking

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

note, this code will move to the new nsMIMEInfoMac.cpp
(uriloader/exthandler/mac) soon (i.e. hopefully before 1.7alpha)

(will happen in bug 78919)

Comment 19

[Mike Pinkerton (not reading bugmail)]

i have a new patch (which i can't diff because cvs is down) but i have no idea
how to test it. the instructions here aren't very informative.

can someone provide a testcase?

Comment 20

[Mike Pinkerton (not reading bugmail)]

Attachment: better merged patch

updated patch with correct merge

Comment 21

[Simon Fraser [no longer active]]

+      FSRef tempFileRef;
+      tempFile->GetFSRef(&tempFileRef);

Does the temp file have the right extension (and file type?) at this point?
Also, a diff -w might be clearer.

Comment 22

[Mike Pinkerton (not reading bugmail)]

Attachment: diff -w of above

diff -w per smfr's request

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

please test this use case though:
o) set some application as the helper application for a file type
o) remove/rename that application
o) load a file with that mime type

something sensible should happen :) I suspect this may cause this new code to be
invoked, but I am not sure... especially I am not sure if it is supposed to be
invoked...

Comment 24

[Steve Dagley]

The oriinal problem was apparently the InternetConfig database of helper apps
would get out of sync with the LaunchServices database.  We'd query IC which
would deny there was a handler for the file type after download but the user
could double-click on the file and LaunchServices would dispatch it.  How you
force that to happen I don't know so I'd test by commenting out the if
(application) block and force the code path thru the LS dispatching code.

Comment 25

[Mike Pinkerton (not reading bugmail)]

landed