386078 - launchWithFile needs to be launchWithURI

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Shawn Wilsher :sdwilsh]

In nsIHanderInfo, we have launchWithFile, but for protocols we will just want to pass the URI in.  We can QI the nsIURI to an nsIFileURI if it is a file, and carry out the old code path if it's a file, or do the new one if that fails.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v1

A first iteration, which changes launchWithFile to launchWithURI, but doesn't yet handle the nsIWebHandlerApp case...

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 270059 [details] [diff] [review]
patch, v1

>-     * @param aFile The file to launch this application with.
>+     * @param aFile The URI to launch this application with; this may or may
nit: aURI

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2 (still in progress)

Doesn't work with web handlers let, but should work with local handlers.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: change launchWithFile to launchWithURI

Just a few cleanups.  Since this is separable from making LaunchWithWebHandler work, let's land the smallest patch possible.

Comment 5

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

Comment on attachment 271273 [details] [diff] [review]
change launchWithFile to launchWithURI

netwerk/mime/public/nsIMIMEInfo.idl
+     * @param aURI The URI to launch this application with; this may or may
+     *              not be a local file or may not.

there's an "may not" too much here :)

uriloader/exthandler/nsMIMEInfoImpl.cpp
+  nsCOMPtr<nsILocalFile> localFile;
+  nsresult rv;

why move rv here?

Also, I can't quite figure out why this code works. You never seem to initialize this localFile variable.

+    nsCOMPtr<nsILocalFile> docToLoad = do_QueryInterface(file, &rv);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    return LaunchWithIProcess(executable, localFile);

that localFile should be docToLoad, right?

+    nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(aURI, &rv);

and this can't possibly work, can it?


Have you tested this?

+      nsCOMPtr<nsIWebHandlerApp> webHandler;
+      webHandler = do_QueryInterface(mPreferredApplication, &rv);

is there a particular reason why you didn't do this in one statement?

uriloader/exthandler/mac/nsMIMEInfoMac.cpp
+  nsCOMPtr <nsILocalFileMac> tempFile = do_QueryInterface(aURI, &rv);

again, this won't work

+    nsCOMPtr <nsILocalFile> docToLoad = do_QueryInterface(aURI, &rv);

and here

uriloader/exthandler/os2/nsMIMEInfoOS2.cpp

since there's no aFile anymore, you'll have to update references to it (changing them to localFile/file)

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #5)

> uriloader/exthandler/nsMIMEInfoImpl.cpp
> +  nsCOMPtr<nsILocalFile> localFile;
> +  nsresult rv;
> 
> why move rv here?

If I didn't I'd have to declare a separate rv for use in the else clause.

[remaining lossage elided]

My Windows VM has been horked; so I didn't get a chance to test on Windows, which would have caught much of the remaining issues.  I shouldn't have asked for review until I'd fixed that; my apologies.  I'll do that, then address any remaining issues and make sure the code matches on the remaining platform before resubmitting.


> Also, I can't quite figure out why this code works. You never seem to
> initialize this localFile variable.
> 
> +    nsCOMPtr<nsILocalFile> docToLoad = do_QueryInterface(file, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    return LaunchWithIProcess(executable, localFile);
> 
> that localFile should be docToLoad, right?
> 
> +    nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(aURI, &rv);
> 
> and this can't possibly work, can it?
> 
> 
> Have you tested this?

No, this code works on
> 
> +      nsCOMPtr<nsIWebHandlerApp> webHandler;
> +      webHandler = do_QueryInterface(mPreferredApplication, &rv);
> 
> is there a particular reason why you didn't do this in one statement?
> 
> uriloader/exthandler/mac/nsMIMEInfoMac.cpp
> +  nsCOMPtr <nsILocalFileMac> tempFile = do_QueryInterface(aURI, &rv);
> 
> again, this won't work
> 
> +    nsCOMPtr <nsILocalFile> docToLoad = do_QueryInterface(aURI, &rv);
> 
> and here
> 
> uriloader/exthandler/os2/nsMIMEInfoOS2.cpp
> 
> since there's no aFile anymore, you'll have to update references to it
> (changing them to localFile/file)
> 

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

[or not so much elided; sigh]

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v4

This hoists the nsIURI -> nsILocalFile pattern out into its own static method.  Since MacOS is _always_ OS X now, I remove the  ifdef in nsMIMEInfoMac.cpp and also clean up that function so that it was easier to convince myself that the code paths were doing the right thing.  I did more testing on Mac, and tested on Linux as well (which shares the Windows code), so we should be in good shape now.

Comment 9

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

Comment on attachment 271899 [details] [diff] [review]
patch, v4

nsMIMEInfoImpl.cpp:
+nsMIMEInfoBase::GetLocalFileFromURI

that function uses 4-space indentation, while the rest of the file has 2-space... please fix :)

     NS_ENSURE_SUCCESS(rv, rv);
-        
+    
     nsCOMPtr<nsIFile> executable;

when you touch whitespace-only lines, just remove all the whitespace


Hm... I wonder if LaunchDefaultWithFile should be changed to take a URI... I think on GNOME, the system API allows passing URIs to the app. (just a random thought, you don't have to change this now)

+++ uriloader/exthandler/nsMIMEInfoImpl.h	10 Jul 2007 22:35:07 -0000	1.27.4.6
+    static NS_HIDDEN_(nsresult) LaunchWithWebHandler(nsIWebHandlerApp *aApp, 
+                                                     nsIURI *aUri);

+    static NS_HIDDEN_(nsresult) GetLocalFileFromURI(nsIURI *aURI,
+                                                    nsILocalFile **aFile);
+

Can you use a consistent spelling for aURI? :)

+     * @return       the associated nsILocalFile

that local file is actually an out param, not a return value here

+++ uriloader/exthandler/mac/nsMIMEInfoMac.cpp	11 Jul 2007 20:18:34 -0000	1.5.86.9
+    nsCOMPtr<nsIWebHandlerApp> webHandlerApp =
+        do_QueryInterface(mPreferredApplication, &rv);

That file uses 2-space indentation...

-      do_QueryInterface(mPreferredApplication, &rv);
+        do_QueryInterface(mPreferredApplication, &rv);

here too

+      app = (do_CreateInstance("@mozilla.org/file/local;1"));

no need for these parentheses around do_CI

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #9) 
> Hm... I wonder if LaunchDefaultWithFile should be changed to take a URI... I
> think on GNOME, the system API allows passing URIs to the app. (just a random
> thought, you don't have to change this now)

Yeah, I suspect so.  We may actually want to turn the system default into a special nsIHandlerApp object of some sort, rather than having a bunch of separate attrs on the nsIHandlerInfo object.

Sorry for the whitespace lossage; I'm dreaming of the day when someone teaches Eclipse about modelines.

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v5

Comments addressed; updated to trunk.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Patch landed:

Checking in netwerk/mime/public/nsIMIMEInfo.idl;
/cvsroot/mozilla/netwerk/mime/public/nsIMIMEInfo.idl,v  <--  nsIMIMEInfo.idl
new revision: 1.28; previous revision: 1.27
done
Checking in uriloader/exthandler/nsExternalHelperAppService.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsExternalHelperAppService.cpp,v  <--  nsExternalHelperAppService.cpp
new revision: 1.315; previous revision: 1.314
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.cpp;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v  <--  nsMIMEInfoImpl.cpp
new revision: 1.58; previous revision: 1.57
done
Checking in uriloader/exthandler/nsMIMEInfoImpl.h;
/cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v  <--  nsMIMEInfoImpl.h
new revision: 1.29; previous revision: 1.28
done
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.cpp;
/cvsroot/mozilla/uriloader/exthandler/mac/nsMIMEInfoMac.cpp,v  <--  nsMIMEInfoMac.cpp
new revision: 1.7; previous revision: 1.6
done
Checking in uriloader/exthandler/mac/nsMIMEInfoMac.h;
/cvsroot/mozilla/uriloader/exthandler/mac/nsMIMEInfoMac.h,v  <--  nsMIMEInfoMac.h
new revision: 1.4; previous revision: 1.3
done
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.cpp;
/cvsroot/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp,v  <--  nsMIMEInfoOS2.cpp
new revision: 1.5; previous revision: 1.4
done
Checking in uriloader/exthandler/os2/nsMIMEInfoOS2.h;
/cvsroot/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.h,v  <--  nsMIMEInfoOS2.h
new revision: 1.4; previous revision: 1.3
done

Comment 13

[Walter Meinl]

Attachment: OS/2 build unbreak

This should unbreak the OS/2 build

nsMIMEInfoOS2.cpp
E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:67: error: prototype
   for `nsresult nsMIMEInfoOS2::LaunchWithURI(nsIFile*)' does not match any in 
   class `nsMIMEInfoOS2'
E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.h:49: error: candidate
   is: virtual nsresult nsMIMEInfoOS2::LaunchWithURI(nsIURI*)
E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp: In member 
   function `nsresult nsMIMEInfoOS2::LaunchWithURI(nsIFile*)':
E:/usr/src/mozilla/uriloader/exthandler/os2/nsMIMEInfoOS2.cpp:71: error: no 
   matching function for call to `nsMIMEInfoOS2::GetLocalFileFromURI(nsIFile*&, 
   nsGetterAddRefs<nsILocalFile>)'
E:/usr/src/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h:155: error: candidates 
   are: static nsresult nsMIMEInfoBase::GetLocalFileFromURI(nsIURI*, 
   nsILocalFile**)
make.exe[5]: *** [nsMIMEInfoOS2.o] Error 1

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 272856 [details] [diff] [review]
OS/2 build unbreak

r+sr=dmose

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Bustage fix checked in; thanks for the patch, Walter!