launchWithFile needs to be launchWithURI

RESOLVED FIXED in mozilla1.9alpha8

Status

RESOLVED FIXED
12 years ago
3 years ago

People

(Reporter: sdwilsh, Assigned: dmose)

Tracking

Trunk
mozilla1.9alpha8

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
Flags: blocking1.9?
(Assignee)

Comment 1

12 years ago
Created attachment 270059 [details] [diff] [review]
patch, v1

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

Comment 2

12 years ago
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
(Reporter)

Updated

12 years ago
Target Milestone: --- → mozilla1.9beta1
(Assignee)

Comment 3

12 years ago
Created attachment 270818 [details] [diff] [review]
patch, v2 (still in progress)

Doesn't work with web handlers let, but should work with local handlers.
Attachment #270059 - Attachment is obsolete: true
(Assignee)

Comment 4

12 years ago
Created attachment 271273 [details] [diff] [review]
change launchWithFile to launchWithURI

Just a few cleanups.  Since this is separable from making LaunchWithWebHandler work, let's land the smallest patch possible.
Attachment #270818 - Attachment is obsolete: true
Attachment #271273 - Flags: superreview?(cbiesinger)
Attachment #271273 - Flags: review?(cbiesinger)
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)
Attachment #271273 - Flags: superreview?(cbiesinger)
Attachment #271273 - Flags: review?(cbiesinger)
Attachment #271273 - Flags: review-
(Assignee)

Comment 6

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

(Assignee)

Comment 7

12 years ago
[or not so much elided; sigh]
(Assignee)

Comment 8

12 years ago
Created attachment 271899 [details] [diff] [review]
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.
Attachment #271273 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #271899 - Flags: superreview?(cbiesinger)
Attachment #271899 - Flags: review?(cbiesinger)
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
Attachment #271899 - Flags: superreview?(cbiesinger)
Attachment #271899 - Flags: superreview+
Attachment #271899 - Flags: review?(cbiesinger)
Attachment #271899 - Flags: review+
(Assignee)

Comment 10

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

Comment 11

12 years ago
Created attachment 272711 [details] [diff] [review]
patch, v5

Comments addressed; updated to trunk.
Attachment #271899 - Attachment is obsolete: true
Attachment #272711 - Flags: superreview+
Attachment #272711 - Flags: review+
(Assignee)

Comment 12

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 13

12 years ago
Created attachment 272856 [details] [diff] [review]
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
(Assignee)

Comment 14

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

r+sr=dmose
Attachment #272856 - Flags: superreview+
Attachment #272856 - Flags: review+
(Assignee)

Comment 15

12 years ago
Bustage fix checked in; thanks for the patch, Walter!
(Reporter)

Updated

11 years ago
Flags: blocking1.9?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.