nsIMIMEInfo uses an nsIFile for preferredApplicationHandler This means that: 1) A full pathname must be used in preferences for the application. No $PATH resolution is done (bug 56662) 2) No arguments can be passed to the application (bug 57420) 3) There is no way to support the standard Unix mailcap syntax for application handlers, which allows arbitrary shell expressions. Eg "grep foo %s | lpr" would be a perfectly valid application handler, where %s is replaced by the temp filename. (bug 52441) Maybe create a new interface (nsIPreferredContentHandler) and use that in nsIMimeInfo. That would allow us to be a lot more flexible about what we support...

Adding dependencies. This is pp and 4xp -- we fully support the system ways of doing helpers on Mac and Win and 4.x works just fine on Linux. Nominating for 1.0 -- this is core functionality and we should try to at least get the architecture right for 1.0 even if we do not fix the various bugs blocked by this. ccing mscott. ccing mpt since he expressed an interest in this stuff.

Are you sure about mozilla1.0? Personally, I believe that this is such a major annoyance, that it might be reasoable to try to have it fixed by, say, 0.9.2 so that all the dependent bugs could be fixed for 1.0.

OK. After some more research we have: 1) nsIFile is no good on Unix. We have to be able to store a semi-arbitrary string as a helper 2) a string is no good on Mac. A single path may well point to two different files. This means that on Mac we _have_ to use nsIFile or something along those lines. I'm not sure anymore that the idea of creating an nsIFile-derived thing is such a great one. It may be better to have a totally different class that has two member variables -- an nsIFile, and a string that was used to initialize the nsIFile... ccing dougt. Doug, is there a simpler solution I'm missing?

Is there any reason that this interface needs to be XP? Why not have a UNIX only interface so that we can set mailcap syntax?

1) XP is a good idea in general because there is an XP concept of "get me the thing to do with this mime type". If it simplifies to an nsIFile on some platforms, it still seems to me that it should be an XP wrapper around an nsIFile because there are XP *semantics* involved. 2) Windows also supports command-line parameters, even if you don't see them terribly often. Take a look at the shortcut in your start menu for 4.x mail - it says '"c:\path\to\netscape.exe" -mail'. I guess it's rare on windows for command-line parameters to be necessary for mime-type support, but the base OS supports it, and Mozilla should too. 3) Mac OS X, being unix, will also support command-line parameters. That means it may need a combination of the two sets of semantics, perhaps? This is a big ugly thing that can't be platform-specific-ifdeffed away... Btw, dougt, this isn't only for mailcap parsing, but also for the helper app UI; see bug 57420 (no support for helper app command-line args).

take a look at netscape3's registry info for real player. it heavily abuses command line switches for mime types. [Yes I don't like the fact that realplayer is using the old fassioned n3 entries when it should just use system entries, but that's far afield.]

this is more of a URI loader issue and not necessarily critical for 0.9.1... but I'd let him decide.

mass move, v2. qa to me.

nsIHelperAppLauncher also assumes the helper app is an nIFile... this may or may not be a good assumption there...

I don't think I understand why this can't be a string. Something about it needing to be an nsIFile on MacOS <10? I guess I don't see why a single path may point to more than on thing on a Mac. Sorry, been a while since I've been deep into MacOS programming (but I have been, so I at least think I *should* understand this) It's becomming really annoying for me because of this blocking bug #57420. I'd really like to be able to "properly" invoke helper app's that need command line args. Is there strong opposition to maknig an XP wrapper that can contain either the string (including %s's and such a la NS 4.x) or an nsIFile if, in fact, that is needed for the Mac? I guess it'll be easier, if such a thing is constructed, to tear apart the input string into some sort of nsIFile holding the application, and references (in some form) for the arguments and position for the temp file name. Later, this can be improved so that if the input string isn't a fully qualified path, you could do that path work when going to fill in the nsIFile... Anyway. Mostly just looking into current state and feelings...

the simple answer is that you can have two drives with the same name. absolute paths on macos (which are absolutely taboo) are of the form <volumename>:<folderpath>:<file> where <folderpath> is of the form ""|<folder>:<folderpath> secondly, i should be able to tell mozilla to use DDE or AppleScript or BMessages and i can't do any of those w/ a commandline. Heck on macos there is no commandline so the appropriate way to launch something w/ parameters would probably be applescript, and not whatever you thought you were suggesting. (it's true you can just stick a file onto the stack and pray it behaves, but i don't think you can stick a url like that). so for the grand scheme of things, (and i'm 99% sure i'm confusing bugs, but i'm tired -- sorry) this what i think i'm reading isn't good enough.

Right. Someone else explained that to me off-line. Thanks. And, aren't Apple Events still the way to invoke things on the mac? An OAPP (or ODOC if you need to specify the doc to open) event? If that's what you meant my AppleScript, then I apologize. Haven't worked with AppleScript myself... Anyway, no need for furthur spam/commentary in the bug. I've already been straightened out. :-)

Over to file handling.... I doubt mscott is ever going to even look at this. I'm still thinking of doing this myself at some point, but not till summer....

Future-ing.

(marking All/All, since this seems to be an architecture bug, therefore affects all platforms) Wouldn't the easiest solution be something along these lines: o HelperAppService gets a method "findHelperApp(mimeType)" or something like that, it probably has such a method o the resulting interface (let's call it nsIHelperApp) has a method like this: run(nsIFile aFile); which launches the helper with the file. and maybe an attribute "wstring humanReadableDesc" In other words: The caller does not care at all how the helper app is invoked. It just knows that the interface can somehow run the helper with all the platform-specific stuff it has to do, and with the platform-specific stuff stored in it. Comments on that approach? Also, if what I said above is complete nonsense given how this stuff currently works, please tell me, and I'll do some reading of how this stuff works and make a better suggestion :)

Yeah, that's sorta how things would need to work... Which still means we need an nsIHelperApp which can be usefully created somehow... from outside the helper app service. (Eg the filepicker on the helper app dialog needs to be able to do it.)

Well, you could do it like this: give it a contract id and Init(nsIFile) function and use that from filepicker. have the HelperAppService use a normal C++ constructor or something like that to initialize it. _or_, give it two Init functions, one taking a file, another one taking a mimetype. The former would just use that file as helper app, the latter would look it up from the helperappservice. I would prefer the first approach, I guess.

Yeah... how does this differ from what I suggest in comment 0? ;)

bz: hey, you didn't mention how the interface would look like or be used :) taking bug

Another approach (the current Windows one) is to allow the "system" application to be null and only set the app description. Then when LaunchWithApplication is called with a null app we do whatever it is we need to do (in the case of Windows, ShellExecute the file being viewed). This _could_ be extended to other OSes if we specify in nsIMIMEInfo that that is what should happen... That would, of course, mean that the only thing that could launch the app would be the external helper app service. Not necessarily a good idea, imo.... So yeah, an nsIHelperApp that can be inited with an nsIFile, that encapsulates its description, etc would be great. We could have an XP superclass doing all the nsIFile/description stuff and having a Launch(in nsIFile) impl, and then subclass it as needed on platforms that need special stuff to happen in Launch(). I'd say that the contractid should give an instance of the base class, but maybe we want to ifdef the component registration... Anyway, I'd love to see what you come up with. Thanks for doing this!

status update: linux (well, unix) part of the patch done if anyone is interested, I can attach a work-in-progress.

"yes" ;)

ok, I hope I diffed all changed files please let me know if this fails to compile Note: nsMIMEInfoImpl got some changes which are probably not very visible in this patch, sorry about that, I suggest a manual "diff -u" between the currnet MIMEInfoImpl and the one in this patch.

oh, and what this patch does _not_ do: actually subclass the mimeinfo implementation for good mailcap support that's partly because I don't really know how the mailcap stuff works :) But this patch does make it possible to subclass mimeinfoimpl to easily add such support. that's bug 83305 I guess.

OK, based on a quick skim I have one question... you only use launchWithFile to mean "launch using the default handler"? Is that the plan? Is the code that launches with the "preferred" handler sufficiently similar across the OSes that it could be moved into the mimeinfo base class impl?

bz: Indeed, that was my plan. That's not set in stone, of course. I'll check how similar the other code is. If it is very similar, we can probably get rid of LaunchAppWithTempFile (and replace it with mimeInfo->LaunchWithFile(...)), which I would like, I guess.

Yeah... I really don't like the nsIHelperAppLauncher interface, to be truthful; anything we can do to clean it up would be great.

errr that has its own _interface_? oh indeed... Anyway, I had a short look at the implementations: o Unix just creates an nsIProcess and runs it o Windows and BeOS does the same, except if the file should be opened with the default app, in which case nsILocalFile::Launch is called, unless the file is executable (imho, that executable check does not belong here, but ok...) o Mac uses nsILocalFileMac to launch the file o OS/2 does the same as Unix, with the addition of a few lines of OS/2 specific black magic. Now, that stuff is mostly the same no matter if the default app or preferred app should be used. So just using LaunchWithFile for both default app and preferred app sounds like a good solution to me. It will require some ifdefs, at least until this is actually subclassed, and maybe even then.

> imho, that executable check does not belong here It's a last-ditch check. There are checks before this point, so this check should never be needed, but it's there "just in case". Paranoia is good in the security business. I was thinking we could have the superclass handle launching of "preferred" handlers (since that sounds like it's pretty uniform across platforms, except mac) and have the subclasses handle the "default" handler.... But yes, at first we may have to ifdef it (though I would prefer we did this right to start with).

oh, I see, so every platform would have its own MIMEInfoImpl? Hm, that probably sounds like a good idea. That idea hadn't occured to me, I was only thinking of one impl. for nsIFile-based launching, and another one for string-based launching (like for mailcap)

> oh, I see, so every platform would have its own MIMEInfoImpl? Yep. Exactly. The base class could do some default stuff and then platforms could override as needed.

I uploaded a plan for implementing this at: http://stud4.tuwien.ac.at/~e0225227/plan78919.txt

Comment on attachment 127208 [details] [diff] [review] part 1 (checked in) r=me on all but the ldap changes... ;)

Comment on attachment 127208 [details] [diff] [review] part 1 (checked in) er... don't know how the ldap changes got in there... please ignore them, I won't check them in

Comment on attachment 127208 [details] [diff] [review] part 1 (checked in) sr=darin

Comment on attachment 127208 [details] [diff] [review] part 1 (checked in) part 1 checked in.

Comment on attachment 130028 [details] [diff] [review] Part 2 bz, feel free to take your time with this review. I don't want to land this before 1.6alpha anyway. sigh, this includes the patch for bug 120045... tell me if you'd rather have a patch without that other patch mixed in

No need to split those changes out...... I'll try to get to this, but only once I have a non-modem net connection (could be a week or two).

Re: attachment 130028 [details] [diff] [review] Defining BIN extensions as a binary executable on the Windows platform might not be a good idea. A lot of Windows CD imaging programs create CD images as BIN files. I don't think there is a such thing as an executable BIN file anyway (on Windows).

Jerry, that has nothing to do with this bug; that's the part of the patch that ended up in there by mistake as pointed out in comment 39 (and in fact does not change the existing behavior at all; if you feel that the existing behavior needs changing, feel free to file a separate bug on that).

biesi, could you attach a patch against current trunk when you get a chance? I'll try to review this tomorrow (that's about 18 hours from now)...

updated to trunk

Comment on attachment 132608 [details] [diff] [review] Part 2, v2 >Index: nsExternalHelperAppService.cpp >+ aMIMEInfo->SetApplicationDescription(nsnull); >+ aMIMEInfo->SetPreferredApplicationHandler(nsnull); Add a brief comment as to why the two lines are there? I'm not sure why they are needed... > if (NS_SUCCEEDED(rv) && exists) ... >+ else if (NS_SUCCEEDED(rv) && !exists) { "else if (NS_SUCCEEDED(rv))" is enough (since if "exists" is also true, you would never make it to the else here). >+already_AddRefed<nsIMIMEInfo> nsExternalHelperAppService::GetMIMEInfoFromOS(const char * aContentType, const char * aFileExt, PRBool& aFound) Please use a pointer for an out param... (that makes it clear that it is in fact an out param at the caller end of things). > NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(const char *aMIMEType, const char *aFileExt, nsIMIMEInfo **_retval) >+ rv = GetMIMEInfoForMimeTypeFromDS(aMIMEType, *_retval); This is only safe because the DS will not override the info we want from the OS. comment to that effect. Also, the DS _will_ override the description with an empty string.... In FillTopLevelProperties, we want to only set the description if it's nonempty. >+ if (NS_FAILED(rv)) { > // No type match, try extension match > nsCOMPtr<nsIMIMEInfo> dsInfoExt; dsInfoExt is no longer used, right? >+ if (!found) { found will be false if no OS-level info for this type. But there may be info from the DS, no? You probably want (!found && NS_FAILED(rv)) or something to that effect... Then you can probably just pass in _retval as the MIME info, and not have to do that GetMIMEInfoFromOS thing in the *Extras functions.... >+ if (LOG_ENABLED()) { That should be in #ifdef PR_LOGGING > nsresult nsExternalHelperAppService::GetMIMEInfoForExtensionFromExtras(const char* aExtension, nsIMIMEInfo ** aMIMEInfo ) >+ nsCOMPtr<nsIMIMEInfo> mimeInfo = >+ GetMIMEInfoFromOS(extraMimeEntries[index].mMimeType, >+ nsnull, ignored); How come this is done like this, while the similar Type function just assigns to the out param? >Index: nsExternalHelperAppService.h >+ * @param aFound "@param aFound [out]" or some such (not sure what the right javadoc syntax is) >+ * @return A MIMEInfo. This function is supposed to always return a MIMEInfo; >+ * should only be returned in failures like out-of-memory. "null should only be returned"? Maybe something more like: "This function must return a MIMEInfo object if it can allocate one. The only justifiable reason for not returning one is an out-of-memory error." >Index: os2/nsOSHelperAppService.cpp > nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType, >+ // If we found nothing, and had no type, just return >+ if (!retval) >+ return nsnull; The only way retval can be null there is if we ran out of memory (or were otherwise unable to instantiate the contractid). Should this be testing aType or something? Or should this test just not exist? >Index: unix/nsOSHelperAppService.cpp Same issue.

>The only way retval can be null there is if we ran out of memory (or were >otherwise unable to instantiate the contractid). Should this be testing aType >or something? Or should this test just not exist? hm, that's not true actually. this seems unreachable. (the |return retval| above is not inside an if (retval)) >You probably want (!found && NS_FAILED(rv)) or something to >that effect... I did it differently and set found to true if getting something from the ds worked attaching a new patch in a minute, once I get it to compile...

Comment on attachment 134162 [details] [diff] [review] part 2, v3 >Index: nsExternalHelperAppService.cpp > NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(const char *aMIMEType, const char *aFileExt, nsIMIMEInfo **_retval) >+ PRBool found; >+ *_retval = GetMIMEInfoFromOS(aMIMEType, aFileExt, &found).get(); >+ if (aMIMEType && *aMIMEType) { >+ rv = GetMIMEInfoForMimeTypeFromDS(aMIMEType, *_retval); >+ found = NS_SUCCEEDED(rv); > } >+ if (!found) { >+ rv = GetMIMEInfoForExtensionFromDS(aFileExt, *_retval); >+ found = NS_SUCCEEDED(rv); > } > } > > // (3) No match yet. Ask extras. >+ if (!found) { >+ rv = NS_ERROR_FAILURE; So if we get an OS-level match but don't find the type/extension in our DS we will hit the extras? That seems wrong... I think we should only hit the extras if there is nothing from the OS or from the DS at all for that type/extension. >Index: os2/nsOSHelperAppService.cpp > already_AddRefed<nsIMIMEInfo> > nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType, >+ // If we found nothing, and had no type, just return >+ if (!retval) >+ return nsnull; Like you said, this return is unreachable. Please remove it? >Index: unix/nsOSHelperAppService.cpp > nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType, >+ // If we found nothing, and had no type, just return >+ if (!retval) >+ return nsnull; Same. r=bzbarsky with those changes. I'm sorry it took me so long to get to this... :(

ok, I changed the code to: if (aMIMEType && *aMIMEType) { // This will not overwrite the OS information that interests us // (i.e. default application, default app. description) rv = GetMIMEInfoForMimeTypeFromDS(aMIMEType, *_retval); found = found || NS_SUCCEEDED(rv); } so if Get...FromDS fails, but found was true, found will stay true. will attach a new patch in a second

So if we get OS-level info, but don't get anything out of the DS based on the type we will skip looking in the DS based on the extension? That's wrong too....

Comment on attachment 135066 [details] [diff] [review] part 2, v4 you're correct...

Comment on attachment 135073 [details] [diff] [review] part 2, v5 (checked in) + NS_ENSURE_ARG_POINTER( aExtension ); + NS_ENSURE_ARG( *aExtension ); how about using NS_ASSERTION instead? we shouldn't have to null check input args like this. + if (!miByExt && !mi) { + *aFound = PR_FALSE; + CallCreateInstance(NS_MIMEINFO_CONTRACTID, &mi); + if (mi) { + if (aMIMEType && *aMIMEType) + mi->SetMIMEType(aMIMEType); + if (aFileExt && *aFileExt) + mi->AppendExtension(aFileExt); + } this blob of code seems to be added to all the platform specific files. should it be factored into the cross- platform code? sr=darin

Comment on attachment 135073 [details] [diff] [review] part 2, v5 (checked in) part 2 v5 checked in

move nsMIMEInfoImpl from netwerk/mime/src to uriloader/exthandler; create it using |new nsMIMEInfoImpl| instead of using the component manager. this does not allow creating mimeinfos via CreateInstance anymore. (nothing in cvs.mozilla.org depends on that, except exthandler and that's patched here).

oh yeah, I will of course get nsMIMEInfoImpl.* to be repository-copied. I know that for bug 190413, people need a way to create mimeinfos to pass it to the nsIXMLMIMEService; but they can just ask the mimeservice for one.

well, this is the missing netwerk/mime/src/Makefile.in change.

Comment on attachment 139620 [details] [diff] [review] part3 v1 addition (checked in) r=bzbarsky

Comment on attachment 139613 [details] [diff] [review] part 3 v1 (checked in) >Index: uriloader/exthandler/beos/nsOSHelperAppService.cpp >+ nsCOMPtr<nsIMIMEInfo> mimeInfo = new nsMIMEInfoImpl; new nsMIMEInfoImpl(), please (just makes it clearer that a constructor call happens there.... ;)) Same in the other places where you have |new nsMIMEInfoImpl|. r=bzbarsky with that.

Comment on attachment 139613 [details] [diff] [review] part 3 v1 (checked in) ok, I will make that change

Comment on attachment 139613 [details] [diff] [review] part 3 v1 (checked in) sr=darin

Comment on attachment 139620 [details] [diff] [review] part3 v1 addition (checked in) sr=darin

OK, so the following files need to be moved in cvs: netwerk/mime/src/nsMIMEInfoImpl.cpp netwerk/mime/src/nsMIMEInfoImpl.h To: uriloader/exthandler, unchanged name darin: could you give sr for that move?

sr=darin on the move

Done. dave@emac [21:58 exthandler 29] tcsh> cvs -q stat -v nsMIMEInfoImpl.cpp =================================================================== File: nsMIMEInfoImpl.cpp Status: Up-to-date Working revision: 1.46 Repository revision: 1.46 /cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.cpp,v Sticky Tag: (none) Sticky Date: (none) Sticky Options: (none) Existing Tags: No Tags Exist dave@emac [21:58 exthandler 30] tcsh> cvs -q stat -v nsMIMEInfoImpl.h =================================================================== File: nsMIMEInfoImpl.h Status: Up-to-date Working revision: 1.17 Repository revision: 1.17 /cvsroot/mozilla/uriloader/exthandler/nsMIMEInfoImpl.h,v Sticky Tag: (none) Sticky Date: (none) Sticky Options: (none) Existing Tags: No Tags Exist You may cvs remove the old versions whenever you're ready.

Comment on attachment 139613 [details] [diff] [review] part 3 v1 (checked in) part 3 checked in, including removal of old nsMIMEInfoImpl.{cpp,h}

this basically gives each os its own nsMIMEInfo implementation. except unix, which uses nsMIMEInfoImpl. Most functions of nsMIMEInfoImpl moved to nsMIMEInfoBase, which doesn't know about default applications. I didn't want to make the windows impl inherit functions (and variables) it had no use for, especially as that may mislead people into thinking they are used. This means that nsMIMEInfoBase has two pure virtual functions - LaunchDefaultWithFile and GetHasDefaultHandler (the latter from nsIMIMEInfo). The new function launchWithFile calls LaunchDefaultWithFile when mPreferredAction is set to useSystemDefault (by default. can be overridden, OS/2 does that, mac too iirc). patch was tested on all affected platforms. testcases: compile, open a file with the default application, open it with a specified application.

Comment on attachment 140758 [details] [diff] [review] part 4 > ? uriloader/exthandler/os2/nsMIMEInfoOS2.cpp > ? uriloader/exthandler/os2/nsMIMEInfoOS2.h CVS add these so I can take a look? ;) >Index: netwerk/mime/public/nsIMIMEInfo.idl >+ * a pretty name description of the default application Mention that this is only usable if hasDefaultHandler is set? >+ * Launches the application with the specified file, depending on the >+ * preferredAction attribute. "in a way that depends on the value of preferredAction" > action must be useHelperApp or >+ * useSystemDefault. "preferredAction must be..." Also, add a @throws line to say what we throw if it's not? >Index: uriloader/exthandler/nsExternalHelperAppService.h >-protected: How come? >Index: uriloader/exthandler/nsMIMEInfoImpl.cpp >-nsMIMEInfoImpl::nsMIMEInfoImpl() { >+nsMIMEInfoBase::nsMIMEInfoBase() { > mPreferredAction = nsIMIMEInfo::saveToDisk; > mAlwaysAskBeforeHandling = PR_TRUE; Make those initializers while you're here? >+nsMIMEInfoBase::nsMIMEInfoBase(const char *aMIMEType) :mMIMEType( aMIMEType ){ > mPreferredAction = nsIMIMEInfo::saveToDisk; > mAlwaysAskBeforeHandling = PR_TRUE; Same. >Index: uriloader/exthandler/nsMIMEInfoImpl.h >+ * be set using the public SetDefaultApplication (not exposed on any interface). >+ * If this is not true for a platform, they should create their own subclass of >+ * this interface I'd say they should subclass nsMIMEInfoBase, not "this interface".... They can still override LaunchWithFile if they had to. > with an appropriate implementation of launchWithFile, or use >+ * nsMIMEInfoBase. They can't "use" it -- it has virtual methods. Also, we're talking about LaunchDefaultWithFile, not LaunchWithFile, are we not? >+ * Alternatively, people can accept the base class's implementation of launching >+ * the preferred application (which is to use LaunchWithIProcess) and just >+ * override LaunchDefaultWithFile, which will be called by launchWithFile if the >+ * action is set to useSystemDefault. Oh, I see. This comment is pretty confusing. I would just tell people to subclass nsMIMEInfoImpl if their default app is an nsIFile, and otherwise to subclass nsMIMEInfoBase and implement LaunchDefaultWithFile... anyone who really needs more will figure out for themselves that they can override virtual functions. ;) >+ * >+ * Another assumption is that hasDefaultApp is true iff mDefaultApplication is >+ * non-null and existant. hasDefaultHandler, no? You never check its existence... Should you? >Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp >-#ifdef XP_MACOSX >- else >- { // We didn't get an application to handle the file from aMIMEInfo, ask LaunchServices directly We probably want to keep that code (and make it actually reachable, in the case when neither preferred nor default app handlers are set.... Don't we have a bug on this code not being reachable already? In any case, this should move into nsMIMEInfoMac::LaunchWithFile (possibly in a followup patch if you prefer). >@@ -266,34 +215,60 @@ nsOSHelperAppService::GetMIMEInfoFromOS( >+ // Copy the attributes of miByType onto miByExt Wouldn't it be simpler to just copy the default app in the other direction? We know what class miByType and miByExt are, so we can do said copy easily, no? May need to add GetDefaultApplication to nsMIMEInfoImpl (but there's no reason not to, right?) I realize those come from the IC service, but maybe we can cheat and cast them? The current code is really a little excessive... On an unrelated note, we should add some calls to SetFileTypeAndCreatorFromMIMEType in the appropriate spots in the helper app code (eg when saving), imo. >Index: uriloader/exthandler/os2/nsOSHelperAppService.cpp >@@ -1514,23 +1404,22 @@ nsOSHelperAppService::GetFromExtension(c >+ nsMIMEInfoOS2* mimeInfo = new nsMIMEInfoOS2(NS_ConvertUCS2toUTF8(mimeType.get()).get()); Why mimeType.get() instead of just mimeType? >@@ -1698,32 +1585,51 @@ nsOSHelperAppService::GetMIMEInfoFromOS( >+ // Copy the attributes of retval onto miByExt, to return it Again, a little silly, esp since this class owns both the method that produced miByExt and the method that produced retval. Just make that method return alredy_AddRefed<nsMIMEInfoOS2> (like you did with Windows), then just copy the default handler and description in this code. >Index: uriloader/exthandler/unix/nsOSHelperAppService.cpp In the usual way, same comments as for OS/2. This time only on >@@ -1644,20 +1594,39 @@ nsOSHelperAppService::GetMIMEInfoFromOS( >+ // Copy the attributes of retval onto miByExt, to return it >Index: uriloader/exthandler/win/nsMIMEInfoWin.h >+ // The default impl of this is wrong for us, as we don't use >+ // mDefaultApplication >+ NS_IMETHOD GetHasDefaultHandler(PRBool * _retval); Ditch the comment; it's not true anyway, since there is no default impl (in the baseclass, which is nsMIMEInfoBase). >Index: uriloader/exthandler/win/nsOSHelperAppService.h >+class nsMIMEInfoWin; >+ already_AddRefed<nsMIMEInfoBase> GetByExtension(const char *aFileExt, const char *aTypeHint = nsnull); Shouldn't the forward decl be for the type we're returning here? I'm surprised this compiled....

Comment on attachment 140817 [details] [diff] [review] part 4: OS/2 mime info impls r=bzbarsky. One more thing. For classes that override LaunchWithFile, perhaps they should also (debug-only) override LaunchDefaultWithFile and put NS_NOTREACHED in it? Just to make sure that none of the callers call the wrong method?

(I'll make the changes that I don't comment on) > (From update of attachment 140758 [details] [diff] [review]) > > ? uriloader/exthandler/os2/nsMIMEInfoOS2.cpp > > ? uriloader/exthandler/os2/nsMIMEInfoOS2.h > > CVS add these so I can take a look? ;) "oops" :) attached. > >Index: uriloader/exthandler/nsExternalHelperAppService.h > > >-protected: > > How come? There is already a protected: line, further up in the file. I saw no point in repeating it. > Oh, I see. This comment is pretty confusing. Yeah, I'll just rewrite it I guess. Originally, there was no nsMIMEInfoBase, and I left the comment pretty untouched. > >+ * Another assumption is that hasDefaultApp is true iff mDefaultApplication is > >+ * non-null and existant. > > hasDefaultHandler, no? details ;) > You never check its existence... Should you? nsIProcess/ShellExecute will fail if it doesn't exist... but true, GetHasDefaultHandler should check. Will make that change. > We probably want to keep that code (and make it actually reachable, in the case > when neither preferred nor default app handlers are set.... Don't we have a > bug on this code not being reachable already? I'd like to keep making this code reachable in bug 183848. > >@@ -266,34 +215,60 @@ nsOSHelperAppService::GetMIMEInfoFromOS( > >+ // Copy the attributes of miByType onto miByExt > > Wouldn't it be simpler to just copy the default app in the other direction? We > know what class miByType and miByExt are, so we can do said copy easily, no? > May need to add GetDefaultApplication to nsMIMEInfoImpl (but there's no reason > not to, right?) This is probably doable on OS/2. But MacOS asks Internet Config... oh, you know that: > I realize those come from the IC service, but maybe we can cheat and cast them? > The current code is really a little excessive... If we had dynamic_cast available, I'd maybe be willing to. As it is, I fear too much that someone would make internet config return another impl of nsIMIMEInfo, and that would be a _bad_ thing if I static_cast this. IC has its own IDL file, so it's hard to change what it returns... scratch that. it turns out I have dynamic_cast on the mac. > On an unrelated note, we should add some calls to > SetFileTypeAndCreatorFromMIMEType in the appropriate spots in the helper app > code (eg when saving), imo. eh, we don't do that already? will file a bug on that. > >@@ -1698,32 +1585,51 @@ nsOSHelperAppService::GetMIMEInfoFromOS( > >+ // Copy the attributes of retval onto miByExt, to return it > > Again, a little silly, esp since this class owns both the method that produced > miByExt and the method that produced retval. actually that's not true. parts come from nsGNOMERegistry. And once real support for mailcap entries is implemented, this code will fall apart - mime infos from gnome registry will probably still be nsMIMEInfoImpl, those from mailcap would be a new subclass of nsMIMEInfoBase. OS/2 could probably just be changed to copy the mailcap state then, as it has no gnomeregistry equivalent... > Just make that method return > alredy_AddRefed<nsMIMEInfoOS2> (like you did with Windows), then just copy the > default handler and description in this code. oh, all of that was about os/2? ah well. I'll make this change for os2. > >Index: uriloader/exthandler/unix/nsOSHelperAppService.cpp > > In the usual way, same comments as for OS/2. see above. > >Index: uriloader/exthandler/win/nsMIMEInfoWin.h > > >+ // The default impl of this is wrong for us, as we don't use > >+ // mDefaultApplication > >+ NS_IMETHOD GetHasDefaultHandler(PRBool * _retval); > > Ditch the comment; it's not true anyway, since there is no default impl (in the > baseclass, which is nsMIMEInfoBase). right... forgot to remove this comment. > >Index: uriloader/exthandler/win/nsOSHelperAppService.h > > >+class nsMIMEInfoWin; > > >+ already_AddRefed<nsMIMEInfoBase> GetByExtension(const char *aFileExt, const char *aTypeHint = nsnull); > > Shouldn't the forward decl be for the type we're returning here? I'm surprised > this compiled.... this has a full decl already, by way of nsExternalHelperAppService.h. I don't use nsMIMEInfoWin because I assign this into a nsRefPtr<nsMIMEInfoBase>. hm, why didn't I make that a nsRefPtr<nsMIMEInfoWin>? Will change this to use nsMIMEInfoWin. (In reply to comment #71) > (From update of attachment 140817 [details] [diff] [review]) > r=bzbarsky. One more thing. For classes that override LaunchWithFile, > perhaps they should also (debug-only) override LaunchDefaultWithFile and put > NS_NOTREACHED in it? Just to make sure that none of the callers call the wrong > method? sounds like a good idea, will do that.

note to self: add a function "public: void CopyBasicData(nsMIMEInfoBase* target)" to nsMIMEInfoBase, for use on os/2,mac,unix

(I filed bug 233375 about setting mac type and creator on downloaded files)

Comment on attachment 140827 [details] [diff] [review] part 4 v2 changes made

Comment on attachment 140827 [details] [diff] [review] part 4 v2 >Index: uriloader/exthandler/nsMIMEInfoImpl.cpp >+nsMIMEInfoImpl::GetHasDefaultHandler(PRBool * _retval) >+{ ... >+ if (NS_SUCCEEDED(mDefaultApplication->Exists(&exists)) && exists) >+ *_retval = PR_TRUE; How about *_retval = NS_SUCCEEDED(mDefaultApplication->Exists(&exists)) && exists; ? >Index: uriloader/exthandler/nsMIMEInfoImpl.h >+ void CopyBasicDataTo(nsMIMEInfoBase* aOther); The impl didn't copy the mac stuff. >Index: uriloader/exthandler/beos/nsMIMEInfoBeOS.h >+class nsMIMEInfoBeOS : public nsMIMEInfoImpl { Could make this inherit from nsMIMEInfoBase, actually... Separate patch, if you want. >Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp >+#include "nsIStringEnumerator.h" Don't need that, do you? >Index: uriloader/exthandler/os2/nsOSHelperAppService.cpp >+#include "nsIStringEnumerator.h" Same. >Index: uriloader/exthandler/unix/nsOSHelperAppService.cpp >+#include "nsIStringEnumerator.h" And here. r=bzbarsky with those nits picked.

all done, except the beos change for which I will file a new bug

Comment on attachment 140830 [details] [diff] [review] part 4 v3 >+ nsAutoString mDescription; // human readable description nsAutoString is not meant to be used as a member variable. why are you doing that? >+ static NS_HIDDEN_(nsresult) talk to bryner... i don't know if we're ready to start using NS_HIDDEN... >+ NS_IMETHODIMP GetDefaultDescription(PRUnichar ** aDefaultDescription); why is this declared NS_IMETHODIMP instead of NS_IMETHOD? seems like you might have a reason... care to share that? > virtual NS_HIDDEN_(nsresult) isn't this what NS_IMETHOD corresponds to? > + * The Initial Developer of the Original Code is + * Paul Ashford. + * Portions created by the Initial Developer are Copyright (C) 2002 + * the Initial Developer. All Rights Reserved. bad license file? or is this really his code? hmm.. BeOS header file has your name as the initial developer. > virtual nsresult LaunchDefaultWithFile why not marked HIDDEN here? more to come....

(In reply to comment #79) > (From update of attachment 140830 [details] [diff] [review]) > >+ nsAutoString mDescription; // human readable description > > nsAutoString is not meant to be used as a member variable. why are you doing > that? Indeed, why am I... Was a mistake I guess, I'll switch to nsString. > >+ static NS_HIDDEN_(nsresult) > > talk to bryner... i don't know if we're ready to start using NS_HIDDEN... bryner tells me it's ok to use it. > >+ NS_IMETHODIMP GetDefaultDescription(PRUnichar ** aDefaultDescription); > > why is this declared NS_IMETHODIMP instead of NS_IMETHOD? seems like you might > have a reason... care to share that? the reason is that I copied the line from the .cpp file and forgot to change this :( will change this to NS_IMETHOD > > virtual NS_HIDDEN_(nsresult) > > isn't this what NS_IMETHOD corresponds to? NS_IMETHOD also uses stdcall (on windows)... here, I don't really care about the calling convention. > > + * The Initial Developer of the Original Code is > + * Paul Ashford. > + * Portions created by the Initial Developer are Copyright (C) 2002 > + * the Initial Developer. All Rights Reserved. > > bad license file? or is this really his code? it is a copy&paste from nsOSHelperAPpService, so this is really his code. > hmm.. BeOS header file has your name as the initial developer. yes - this file is newly created by me. > > virtual nsresult LaunchDefaultWithFile > > why not marked HIDDEN here? no good reason :( (god, was I asleep when I wrote this patch?)

Comment on attachment 140830 [details] [diff] [review] part 4 v3 nit: indentation is funky in nsMIMEInfoMac.cpp > + nsMIMEInfoBase* byType = dynamic_cast<nsMIMEInfoBase*>(miByType.get()); hmm... we don't compile with RTTI support, so is this really of any value? there's a lot of string converions happening in this code block (maybe convert major/minorType just once?): NS_LossyConvertUCS2toASCII(minorType).get(), NS_LossyConvertUCS2toASCII(mime_types_description).get())); if (majorType.IsEmpty() && minorType.IsEmpty()) { // we didn't get a type mapping, so we can't do anything useful return nsnull; } - nsIMIMEInfo* mimeInfo = new nsMIMEInfoImpl(); + mimeType = majorType + NS_LITERAL_STRING("/") + minorType; + nsMIMEInfoOS2* mimeInfo = new nsMIMEInfoOS2(NS_ConvertUCS2toUTF8(mimeType).get()); sr=darin

>hmm... we don't compile with RTTI support, so is this really of any value? mac code does that in other places too, random example: http://lxr.mozilla.org/seamonkey/source/gfx/src/mac/nsPrintSettingsMac.cpp#230 (lxr for dynamic_cast finds a lot more places)

(In reply to comment #81) > there's a lot of string converions happening in this code block (maybe convert > major/minorType just once?): I added a local variable NS_LossyConvertUTF16toASCII asciiMajorType and asciiMinorType to only convert those once. Checked in.

filed Bug 235350 for the beos suggestion from comment 77.