These two methods were coalesced in the patch from bug 386078. The theory was that the callee could just QI to nsILocalFile to decide whether it should be trying to launch by value (LaunchWithFile) or launch by reference (LaunchWithURI). It turns out that this is incorrect, because it's no longer possible for the callee to decide (when being run by a toolkit app such as Thunderbird) whether it should be launching a file: URI with the system file: handler, or to launch a temporary file using the appropriate system mime-type handler. Worse, though, it made the code in LaunchWithURI much more fragile than it already was. So this splits it out into two methods, in preparation for moving both of these methods onto nsIHandlerApp.

Comment on attachment 275490 [details] [diff] [review] patch, v1 >Index: netwerk/mime/public/nsIMIMEInfo.idl >+interface nsILocalFile; Why did you need to add this? >- * Launches the application with the specified URI, in a way that >+ * Launches the application with the specified file, in a way that That seems backwards, as does the comment on launchWithFile later in the patch. It may be worth documenting that for the URI version the handler used will be selected based on the URI (and not the file it points to if it points to a file), while for the file version it will be based on the file... The file:// thing you talk about in the bug, basically. Not sure how to phrase it. >Index: uriloader/exthandler/nsExternalHelperAppService.cpp >+ rv = mMimeInfo->LaunchWithFile(mFinalFileDestination.get()); You don't need the .get() here. >Index: uriloader/exthandler/nsMIMEInfoImpl.cpp >+nsMIMEInfoBase::LaunchWithFile(nsIFile* aDocToLoad) Why not aFile, exactly? >+ // it doesn't make any sense to call this on protocol handlers >+ NS_ASSERTION(mClass == eMIMEInfo, >+ "nsMIMEInfoBase should have mClass == eMIMEInfo"); I anything actually preventing this assert being hit? >+ return LaunchWithIProcess(executable, path); >+ } >+ else if (mPreferredAction == useSystemDefault) { else after return == bad. I'd reverse the if to check for useSystemDefault first (with a short block) and outdent the rest. >+ return LaunchWithIProcess(executable, spec); > >- return LaunchDefaultWithFile(docToLoad); >+ } else if (mPreferredAction == useSystemDefault) { Same here. >Index: uriloader/exthandler/mac/nsMIMEInfoMac.cpp >+nsMIMEInfoMac::LaunchWithFile(nsIFile *aDocToLoad) Again, why not aFile? >+ nsCOMPtr<nsILocalFileMac> lfm = do_QueryInterface(aDocToLoad); Why not just nsILocalFile? sr=bzbarsky with those nits picked.

(In reply to comment #2) > (From update of attachment 275490 [details] [diff] [review]) > >Index: netwerk/mime/public/nsIMIMEInfo.idl > > It may be worth documenting that for the URI version the handler used will be > selected based on the URI (and not the file it points to if it points to a > file), while for the file version it will be based on the file... The file:// > thing you talk about in the bug, basically. Not sure how to phrase it. Yes, good idea. I've added a comment; I'm not thrilled with it, but it's the best I could come up with. > >+ // it doesn't make any sense to call this on protocol handlers > >+ NS_ASSERTION(mClass == eMIMEInfo, > >+ "nsMIMEInfoBase should have mClass == eMIMEInfo"); > > Is anything actually preventing this assert being hit? LaunchWithFile is (and should be) only called by the unknown content-type handler, not the protocol handling dialog. > >+ nsCOMPtr<nsILocalFileMac> lfm = do_QueryInterface(aDocToLoad); > > Why not just nsILocalFile? Because the method that we call (LaunchWithDoc) exists on nsILocalFileMac and not nsILocalFile. All remaining nits addressed.

> Because the method that we call (LaunchWithDoc) exists on nsILocalFileMac and > not nsILocalFile. Yes, but the arg it takes is an nsILocalFile, and we're talking about the arg, not the object the method is being called on. Put another way, that arg was an nsILocalFile before your patch, and I see no reason to change that.

bz: you're totally right; that was a thinko on my part. Fixed patch.

Comment on attachment 275841 [details] [diff] [review] patch, v3 uriloader/exthandler/nsExternalHelperAppService.cpp er, what about the other two callers of LoadWithURI? Don't you have to change them back to WithFile too? uriloader/exthandler/mac/nsMIMEInfoMac.cpp + rv = localHandlerApp->GetExecutable(getter_AddRefs(application)); + NS_ENSURE_SUCCESS(rv, rv); + + } + else if (mPreferredAction == useSystemDefault) { + return LoadUriInternal(aURI); + } + else + return NS_ERROR_INVALID_ARG; + + // so pass the entire URI to the handler. + nsCAutoString spec; + aURI->GetSpec(spec); + return OpenApplicationWithURI(application, spec); OK... this code is confusing. I'd move the last 4 lines of this function into the only branch of the if where it's necessary. As it is, the code kind of looks like the first then-block is missing a return statement

oh, and the indentation in that mac code is also inconsistent.

(In reply to comment #7) > (From update of attachment 275841 [details] [diff] [review]) > uriloader/exthandler/nsExternalHelperAppService.cpp > > er, what about the other two callers of LoadWithURI? Don't you have to change > them back to WithFile too? The one in LaunchWithApplication did need to change; fixed. The one in LoadURI is correct, because it's attempting to launch a protocol handler. The remaining comments have been addressed; new patch forthcoming.

New patch; carrying forward r and sr.

Fix checked in.