GetFromMIMEType on Windows has some major issues: 1) It gets the _first_ extension corresponding to the type and uses that (it would be better to have a GetFromTypeAndExtension() that would take the type and the extension from the URL or whatever and use those to possibly pick a different extension to use)... 2) The mime info it returns has only _one_ extension in it, instead of having all the extensions corresponding to the type.

As long as Mozilla insists on renaming files, bugs are going to continue to be filed. There is a way to do this. When Mozilla is instructed to open a file after download, something like the following should happen: function() { result = GetExtensionFromWindowsMimeDatabase(); FindExecutable("C:\\temp\\bogus" +result, NULL, appToUse); /* I don't know if the file passed to FindExecutable has to actually exist. If not, we're on easy street -- just pass it a bogus file name with the extension we got from the registry query. */ ShellExecute(hWnd, open, appToUse, downloadedFileToRun, NULL, SW_SHOWDEFAULT); --- REFERENCES FindExecutable() - http://msdn.microsoft.com/library/en-us/shellcc/platform/shell/reference/functions/findexecutable.asp

I should clarify what is supposed to happen. When Mozilla is going to open a downloaded file: 1) Take the mime-type from the server and look it up in the Windows Registry to get an associated extension (Mozilla already does this). 2) Now that we have the extension, ask Windows what program should be used to open files with that particular extension. 3) *Now* we can use ShellExecute to execute the program from step #2 and pass it the file we are downloading. This is great because renaming the file is uneccesary. In Windows, the file extension is only relevant when asking the shell to open a file. When passing a file to an application the extension has no meaning. The only potential problem with this approach is that any files associated with Internet Explorer must have the correct extension since IE is part of the shell. I should note that NN4 behaved in similar fashion, except that it created its own MIME-to-application mappings and used those to pass the file to the application. This worked fine.

Jerry, this bug is about the fact that step #1 in comment 2 is fundamentally flawed, since the same type could map to multiple extensions that need to be handled differently. > When passing a file to an application the extension has no meaning. This is certainly not true on non-windows platforms and may not even be true on Windows.... (consider things asking Mozilla to open a file; _we_ certainly care about the extension. So do most image manipulation programs). The "potential problem" you cite is actually the heart of the matter and potentially a huge security risk. In any case, this bug is almost never an issue in the "launch with application" case. It _is_ a huge problem in the "save" case.

Well, I have Adobe Photoshop, Paint Shop Pro, and ACDsee for image programs and not one of them complains when I use file -> Open to open a file named "file.exe" (it's really a gif). They all just open it and display it.

I tried to download a "PDF" file from http://iso-top.biz/pdf_wrapper.php?ReNr=4711&KdNr=0815&Firma=Fa&Name=Name&Adresse1=Adr1&PLZ=01234&Ort=City&Datum=2002-08-27&Versandart=1&Art%5B1%5D%5Bname%5D=Knoppix I did this by left clicking on a link. The pdf_wrapper script sends the following headers: 2 Date: Tue, 27 Aug 2002 14:22:21 GMT 3 Content-Length: 3629 4 Content-Type: application/octet-stream 5 Server: Apache/1.3.20 (Linux/SuSE) mod_throttle/3.0 mod_ssl/2.8.4 OpenSSL/0.9.6b PHP/4.2.2 mod_perl/1.26 mod_layout/1.0 mod_fastcgi/2.2.2 mod_dtcl DAV/1.0.2 6 X-Powered-By: PHP/4.2.2 7 Content-Disposition: attachment; filename=iso-top.de_Rechnung-4711.pdf So, it is application/octet-stream and has a Content-Dispoition header with a suggested filename. Mozilla now suggests to save the file as iso-top.de_Rechnung-4711.pdf.php. That's wrong, because of the trailing .php. When I right click on the link and select Save Link Target As..., Mozilla correctly suggests to save the file as iso-top.de_Rechnung-4711.pdf, without a trailing .php. I tried this with Mozilla 1.1 on Windows NT. I'm filing this as a comment to this bug per comment #345 from bug 120327

You misread bug 120327, comment 345. That comment has foo.php3 going to foo.php3.php. "php3" and "php" are both extensions for the _same_ type, and we should not be fucking that up. That's this bug. Your case has two _different_ types involved. It's bug 164816.

This is affecting my dad -- every time he goes to save a text file, it tries to add .GED on the end because that happens to be the first file extension we find when looking for text/plain files; when he saves a TIFF file with the extension .tif we try to add .tiff onto the end because we look up image/tiff and find .tiff before .tif even though both are in there. My dad finds this most annoying.

*** Bug 169800 has been marked as a duplicate of this bug. ***

Taking. I'm sick of this issue. The only acceptable solution is to merge GetFromExtension and GetFromMIMEType. Darin's suggestion in bug 162116, eg.

nsbeta1+/adt2 per the nav triage team.

*** Bug 189434 has been marked as a duplicate of this bug. ***

Would it be a solution to have a function IsExtensionForMimeType, which returns a boolean given a mimetype and a extension, and checks if the given extension is valid for the given mimetype? if that returns true, the adding of the extension could be skipped.

That would be a solution, but a painful hacky one. It would lead to even _more_ code complication, as opposed to the proposal in bug 162116 which would lead to less code both in the callers and in the MIME service.

well, yeah, but my solution could be easily implemented, while bug 162116 appears far from being fixed (last comment in november last year).

Yeah, well. Many things are "easily implemented" as hacks. This is why the Mozilla code is in the crap state it's in. That said, this does not need a _full_ fix for bug 162116; the dependency is almost the wrong way around, as this bug covers one of the numerous issues that need to be fixed to freeze nsIMIMEInfo. I would estimate that fixing this bug will take about 10-20 hours of coding+testing. I just happen to not have the time because I consider other things more important (the fact that I don't use windows is a factor there, of course). If you're interested, I can give you pretty explicit pointers on the code that needs changing to fix this bug...

bah, I admit that I had a fundamental misunderstanding. The real problem here is that nsIMIMEInfo allows several extensions, but windows fills in only one, right? I thought that nsIMIMEInfo wouldn't _allow_ several extensions.

Yes, that's the real problem. nsIMIMEInfo is being friendly; the windows code in nsOSHelperAppService is just broken.

targeting very optimistically; if someone can do it before then, go for it.

Taking the liberty of tweaking the bug summary for better Bugzilla query discoverability.

Jason, I dunno about the last half of your summary! In my case I'm finding things that /should/ be X.exe being saved as X.exe.mp3 ! Boris, could you post - or mail me directly - the pointers you mention to go digging at this one? No promises, but I'll take a look.

> In my case I'm finding things that /should/ be X.exe being saved as X.exe.mp3 If I'm reading the comments here properly (and understanding the overall picture), I think you're either experiencing a server misconfiguration or, as per comment 6, bug 164816. The "EXE" and "MP3" extensions belong to totally *different* mime types, which has nothing to do with this bug.

The following things would need to be changed, imo: 1) nsIMIMEService.idl -- see discussion in bug 162116; for now we just need a function that takes a MIME type _and_ an extension instead of the current GetFromMIMEType and GetFromExtension 2) nsExternalHelperAppService.h/cpp and the various nsOSHelperAppService implementations -- need to merge GetFromMIMEType and GetFromExtension into a single function. Some of these are pretty simple (eg Linux should be pretty easy, since all helper info is looked up by MIME type). Some are more work. The fix for this bug, in particular, lies in making the Windows version smarter about the extension it uses for the helper app lookup -- it should use the extension from the URL if that matches the content type in question. 3) Callers of nsIMIMEService (both C++ and JS) need to be updated to use the new function instead of GetFromMIMEType and GetFromExtension. All that said, comment 20 describes a situation that's most likely caused by a bogus helper app entry in Mozilla prefs (associating application/octet-stream with the "mp3" extension), not by this bug.....

*** Bug 201644 has been marked as a duplicate of this bug. ***

Sorry, but as a non-programmer, basically, I just want to verify that this is the bug that makes Mozilla completely incapable of appropriately opening files that I pull down off my MS Exchange webmail. I routinely open attachments that are Word and Excel files, and Mozilla thinks they are everything but Word and Excel files. Just now, one Word doc, with .doc extension, was identified as type Application/Octet-stream and saved with a .pdf extension. IT'S A WORD DOCUMENT, for God's sake. I can't believe Mozilla can't understand this. OK, I'll stop ranting. Please just confirm that I don't need to file another bug. Thanks.

What you're seeing is almost certainly bug 189598 (the server sends the attachment as application/octet-stream and you have a helper app entry mapping that type to .pdf files). But yes, this bug could lead to similar behavior in more esoteric cases.

looks like I need to combine GetFromType and GetFromExtension for bug 78919... therefore, taking this bug

here's the .idl change that was missing from the previous patch

+ /* Retrieves an nsIMIMEInfo using both the extension + * and the type of a file. The type is given preference + * during the lookup. null is a valid value for both + * the type and extension. */ + nsIMIMEInfo GetFromTypeAndExtension(in string aMIMEType, in string aFileExt); "null is a valid value for either the type or the extension" (...but not both at the same time, right?) what do you think about coalescing all three of these functions into one? that is, if aMIMEType is null, then isn't GetFromTypeAndExtension just equivalent to GetFromExtension? etc.

darin: yeah, that's what I meant. ok... I'll change the callers and remove GetFromType and GetFromExtension

hmm.. so, should it be called GetFromTypeOrExtension? or maybe just GetMimeType or LookupMimeType?

Well, we still have a GetFromURI....

true... in fact, the reason for GetTypeFromURI is to take advantage of mac file type and creator codes available via nsILocalFileMac. IOW, GetTypeFromURI cannot be implemented in terms of GetTypeFromExtension (though it is on other platforms). perhaps it makes good sense to keep the existing methods as is; *shrug*

Comment on attachment 127375 [details] [diff] [review] GetFromTypeAndExtension. XP and unix part. actually, I noticed some problems with the patches here... I'll fix them and attach a new patch

this patch is missing a line in nsExternalHelperAppService.cpp: PRLogModuleInfo* nsExternalHelperAppService::mLog = nsnull; below the headers please pretend that it's there

well, I noticed a small problem with the previous patch, if the helper app entry has no extensions... apply this after applying the previous patch to fix

To get this patch to compile on Linux I had to forward-declare nsHashtable and nsILineInputStream in the header (since those are passed as function args). In addition, that gives me the warning: /home/bzbarsky/mozilla/debug/mozilla/uriloader/exthandler/unix/nsOSHelperAppService.cpp:52: warning: ` nsresult UnescapeCommand(const nsAString&, const nsAString&, const nsAString&, nsHashtable&, nsACString&)' declared `static' but never defined And indeed, UnescapeCommand is declared as a static method in the header and also as a static function in the cpp; only the method is implemented. The same warning applies to: GetFileLocation, LookUpTypeAndDescription, CreateInputStream, GetTypeAndDescriptionFromMimetypesFile, LookUpExtensionsAndDescription, GetExtensionsAndDescriptionFromMimetypesFile, ParseNetscapeMIMETypesEntry, ParseNormalMIMETypesEntry, LookUpHandlerAndDescription, GetHandlerAndDescriptionFromMailcapFile Going to go read the patches now.

Comment on attachment 127808 [details] [diff] [review] complete patch >Index: embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp File a follow-up bug on nsWebBrowserPersist to pass both type _and_ extension to the GetExtensionForContentType code. >Index: mailnews/base/src/nsMessenger.cpp File a follow-up on nsMessenger.cpp -- they may want to pass in an extension. >Index: netwerk/mime/public/nsIMIMEService.idl >+ /* Retrieves an nsIMIMEInfo using both the extension >+ * and the type of a file. The type is given preference >+ * during the lookup. One of aMIMEType and aFileExt >+ * can be null. */ >+ nsIMIMEInfo GetFromTypeAndExtension(in string aMIMEType, in string aFileExt); How about "at least one of aMIMEType and aFileExt must be non-null and nonempty"? Do we want to list anything here as far as the return value? Like that we guarantee the return value will have aMIMEType as the type (if it was nonempty) and will have aFileExt as the primaryExtension if that's a valid extension for aMIMEType? Not quite sure how much we want to guarantee here.... Perhaps best to just say we will return the "best" MIME info we can. >Index: uriloader/exthandler/nsExternalHelperAppService.cpp >+ mLog = PR_NewLogModule("HelperAppService"); >+ if (!mLog) >+ return NS_ERROR_OUT_OF_MEMORY; >+ This needs to be inside |#ifdef PR_LOGGING|, no? Also, shouldn't this have a if (!mLog) check wrapped around the whole thing? Finally, you need to init mLog to 0 somewhere, no? I can't recall how class statics work wrt them getting initialized.... >+ PR_LOG(mLog, 3, ("HelperAppService::DoContent: mime %s, extension %s\n", >+ aMimeContentType, fileExtension.get())); You may have noticed that I prefer having a macro wrapped around this (in nsOSHelperAppService). Your call, I guess, but it abstracts away which exact pointer you're logging to into the macro, as well as abstracting away the log level.... In any case, PR_LOG_WARN instead of "3". Finally, in my experience "mime '%s'" is much more skimmable in a log than "mime %s" (especially if the string is all weird and broken, eg a MIME type of ", extension" in this case....). This applies throughout. >+ PR_LOG(mLog, 3, ("Gave up, CI'd new mimeinfo %p\n", mimeInfo.get())); Just spell out "created", ok? ;) >+already_AddRefed<nsIMIMEInfo> nsExternalHelperAppService::GetMIMEInfoFromOS(const char * aContentType, const char * aFileExt) > { >- NS_PRECONDITION(aMIMEInfo, "Null out param"); >- *aMIMEInfo = nsnull; > // Should be implemented by per-platform derived classes. >- return NS_ERROR_NOT_IMPLEMENTED; >-} Do all subclasses impl this? If so, could we put a NS_NOTREACHED("Derived classes should implement this") here? >+// nsIMIMEService methods >+NS_IMETHODIMP nsExternalHelperAppService::GetFromTypeAndExtension(const char *aMIMEType, const char *aFileExt, nsIMIMEInfo **_retval) > { How about: NS_PRECONDITION((aMIMEType && *aMIMEType) || (aFileExt && *aFileExt), "Give me something to work with"); here? >+ if (aMIMEType) >+ GetMIMEInfoForMimeTypeFromDS(aMIMEType, getter_AddRefs(dsInfoType)); >+ if (aFileExt && *aFileExt) >+ GetMIMEInfoForExtensionFromDS(aFileExt, getter_AddRefs(dsInfoExt)); How about checking *aMIMEType too? If we get a dsInfoType, we will never need the dsInfoExt; could we do the second get only if necessary? >Index: uriloader/exthandler/nsExternalHelperAppService.h >+#define FORCE_PR_LOG >+#include "prlog.h" >+ You need some more ifdefs here.... see how the Unix nsOSHelperAppService does it. >+ /** Given a mimetype and an extension, looks up a mime info from the os. The mime type is >+ * given preference. */ >+ virtual already_AddRefed<nsIMIMEInfo> GetMIMEInfoFromOS(const char * aContentType, const char * aMimeType); Could you wrap some of this stuff to the happy 80-char boundary while you are here? Also, aContentType and aMimeType are sorta the same thing, no? Should be aFileExt? Add some comments about the fact that they have to be non-null (at least one); if nothing else, say it follows the same conventions as GetFromTypeAndExtension. >+ // NSPR Logging >+ static PRLogModuleInfo* mLog; This needs to be in a #ifdef PR_LOGGING >Index: uriloader/exthandler/beos/nsOSHelperAppService.cpp It looks like you basically inlined the old GetFromExtension and GetFromMIMEType functions, no? If so, could you just declare those functions locally in this class and call them from the new function? It would make the code clearer, imo.... I did not review this part carefully in the meantime. >Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp >+ if (!hasDefault && miByType && miByExt) { Again, could we just get miByExt if it's actually needed? In this case, if (!miByType || !hasDefault). Again, want to check *aMIMEType in addition to aMIMEType. >Index: uriloader/exthandler/os2/nsOSHelperAppService.cpp >-static nsresult >-UnescapeCommand(const nsAString& aEscapedCommand, >- const nsAString& aMajorType, >- const nsAString& aMinorType, >- nsHashtable& aTypeOptions, >- nsACString& aUnEscapedCommand) { >+nsresult >+nsOSHelperAppService::UnescapeCommand(const nsAString& aEscapedCommand, for static class methods, please put a // static on the line before the "nsresult" part. >+already_AddRefed<nsIMIMEInfo> nsOSHelperAppService::GetFromExt(const char *aFileExt) { Spell out "extension"? Like so: already_AddRefed<nsIMIMEInfo> nsOSHelperAppService::GetFromExtension(const char *aFileExt) { >- nsCOMPtr<nsIMIMEInfo> mimeInfo(do_CreateInstance(NS_MIMEINFO_CONTRACTID, &rv)); >- if (NS_FAILED(rv)) >- return rv; >- >+ nsIMIMEInfo* mimeInfo = nsnull; >+ rv = CallCreateInstance(NS_MIMEINFO_CONTRACTID, &mimeInfo); >+ if (NS_FAILED(rv)) >+ return nsnull; >+ What's with the indent shift? >+already_AddRefed<nsIMIMEInfo> nsOSHelperAppService::GetFromType(const char *aMIMEType) { Linebreak before the classname here, please. >+already_AddRefed<nsIMIMEInfo> >+nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType, >+ const char *aFileExt) { >+ if (!miByExt) >+ return retval; >+ if (aType) >+ miByExt->SetMIMEType(aType); >+ if (!retval) { >+ miByExt.swap(retval); >+ return retval; >+ } Shouldn't the aType thing move into the |if (!retval)| block? You probably need to remove the static function decls in the cpp here just like for unix (see my previous comment). >Index: uriloader/exthandler/unix/nsOSHelperAppService.cpp Same comments apply here as for the os2 code. >Index: uriloader/exthandler/win/nsOSHelperAppService.cpp >+ if (aTypeHint) >+ typeToUse.Assign(aTypeHint); >+ else { Put braces around the if body if the else body has them, ok? > // close the key > ::RegCloseKey(hKey); >+ >+ return mimeInfo; Weird indentation stuff starting after that comment line.... >+already_AddRefed<nsIMIMEInfo> nsOSHelperAppService::GetMIMEInfoFromOS(const char *aMIMEType, const char *aFileExt) > { > if (PL_strcasecmp(aMIMEType, APPLICATION_OCTET_STREAM) == 0) { > /* XXX Gross hack to wallpaper over the most common Win32 > * extension issues caused by the fix for bug 116938. See bug > * 120327, comment 271 for why this is needed. Not even sure we > * want to remove this once we have fixed all this stuff to work > * right; any info we get from the OS on this type is pretty much > * useless.... >+ * Just lookup by extension for this filetype. > */ >- return NS_ERROR_FAILURE; >+ aMIMEType = nsnull; > } What happens if both aMIMEType and aFileExt are null after that? Do you want to do that check and bail here? >+ nsCAutoString fileExtension; >+ if (aMIMEType) { >+ // (1) try to use the windows mime database to see if there is a mapping to a file extension >+ // (2) try to see if we have some left over 4.x registry info we can peek at... >+ GetExtensionFromWindowsMimeDatabase(aMIMEType, fileExtension); >+ PR_LOG(mLog, PR_LOG_DEBUG, ("Windows mime database: %s\n", fileExtension.get())); >+ if (fileExtension.IsEmpty()) { >+ GetExtensionFrom4xRegistryInfo(aMIMEType, fileExtension); >+ PR_LOG(mLog, PR_LOG_DEBUG, ("4.x Registry: %s\n", fileExtension.get())); >+ } >+ } OK. This continues to have the same problem that the old code did. For example, say I pass in the "application/msoffice" MIME type and the "xls" extension. The expectation is that we would open it in excel. But the extension that GetExtensionFromWindowsMimeDatabase() gets is "doc", so we proceed to open it in Word. Note that "doc" _does_ map back to our original type, so that's not an issue. What should happen here is that GetExtensionFromWindowsMimeDatabase andGetExtensionFrom4xRegistryInfo should return _lists_ of extensions that match that type, then you want to take the extension in that list that matches aFileExt, or if there is none such take the first one, then use that to do the extension lookup. The typeFromExtEquals function may be superfluous (as in, that check may never fail). But I'm not sure about that; would need to think about it. That, and the usual "should check *aMIMEType too" comment applies. >Index: xpfe/communicator/resources/content/contentAreaUtils.js The callers in this file could easily be updated to make use of our brand new better stuff. Please file a followup bug on that (just file it on me; cc me on the other bugs I asked you to file).

Comment on attachment 127833 [details] [diff] [review] supplimental patch This part looks good. ;)

>and will have aFileExt as the primaryExtension if that's a valid >extension for aMIMEType? The current code does not actually guarantee this... should it? The OS may not treat it as the primaryExtension. >Also, shouldn't this have a if (!mLog) check wrapped around the whole thing? Hm? This is a service, no? Why bother? >Finally, you need to init mLog to 0 somewhere, no? Indeed. please see comment 38. I noticed that only after diffing. > In any case, PR_LOG_WARN instead of "3". OK, I will do that, but I didn't use it because this stuff is not exactly a warning, it's just less detailed debug output than PR_LOG_DEBUG. >say I pass in the "application/msoffice" MIME type and the "xls" >extension. (I'll ignore here that neither .doc nor .xls actually map to that type, and that my system has no mapping for application/msoffice...) If .xls actually maps to application/msoffice, my code would set it as an extension on the mimeinfo. The ExternalHelperAppService would find out that .xls is an extension on the MIMEInfo, and set it as a primary extension. We would therefore save the file as .xls, and open it in MS Excel. Did I miss something? >What should happen here is that GetExtensionFromWindowsMimeDatabase >andGetExtensionFrom4xRegistryInfo should return _lists_ of extensions that >match that type That would require iterating over all subkeys of HKEY_CLASSES_ROOT and checking if the type for each extension matches... it seemed like a slow way to do it. I figured that typeFromExtEquals would make this unneeded. > The typeFromExtEquals function may be superfluous (as in, >that check may never fail). Think application/octet-stream and .txt (ok... bad example, as we ignore an octet-stream type). But say that a cgi script serves a file (and that is has no parameters). You'll have an extension of .php or something, and a type of whatever, say application/zip. You don't want .php in the application/zip mimeinfo. The other comments sound good, will make a new patch with them addressed.

> The current code does not actually guarantee this... should it? For now, let's say no. ;) > Hm? This is a service, no? Why bother? Because morons tend to CreateInstance services.... > Indeed. please see comment 38 Right. Reading classes for me. > but I didn't use it because this stuff is not exactly a warning Feel free to create a separate define that gives it a pretty and meaningful name in this context. ;) > If .xls actually maps to application/msoffice, my code would set it as an > extension on the mimeinfo. Ah, good point. Please clearly document that code then. There are still issues with callers who pass in a MIME type but not an extension (it would be best to give those a real extension list), but I guess we will just need to transition callers away from doing that sort of thing. Note that one of those functions (the NS4x one?) actually _has_ an extension list and then just takes one extension from it. Even fixing that would be good, and should not take too much work. >> The typeFromExtEquals function may be superfluous (as in, >> that check may never fail). Ignore this part; I see what you were doing with it now.

*** Bug 177068 has been marked as a duplicate of this bug. ***

>Note that one of those functions (the NS4x one?) actually _has_ an extension >list and then just takes one extension from it. Even fixing that would be good, >and should not take too much work. I'll file another bug for that

bz's comments addressed (yes... the v6 doesn't match the patches attached here... oh well)

Comment on attachment 127968 [details] [diff] [review] camino patch r=me on the camino change.

Comment on attachment 127965 [details] [diff] [review] patch v6 >Index: modules/libpr0n/decoders/icon/mac/nsIconChannel.cpp > if (!contentType.IsEmpty()) > { >- mimeService->GetFromMIMEType(contentType.get(), getter_AddRefs(mimeInfo)); >+ mimeService->GetFromTypeAndExtension(contentType.get(), nsnull, getter_AddRefs(mimeInfo)); > } > if (!mimeInfo) // try to grab an extension for the dummy file in the url. > { >- mimeService->GetFromExtension(fileExtension.get(), getter_AddRefs(mimeInfo)); >+ mimeService->GetFromTypeAndExtension(nsnull, fileExtension.get(), getter_AddRefs(mimeInfo)); > } How about replacing all that garbage with: if (!contentType.IsEmpty() || !fileExtension.IsEmpty()) mimeService->GetFromTypeAndExtension(contentType.get(), fileExtension.get(), getter_AddRefs(mimeInfo)); >Index: uriloader/exthandler/nsExternalHelperAppService.cpp >+ // (1) Try to find a mime object by looking the mime type/extension "looking at the". >+// Copies the user settings from one mime type to another "one mime info to another". >Index: uriloader/exthandler/beos/nsOSHelperAppService.cpp >-NS_IMETHODIMP nsOSHelperAppService::GetFromExtension(const char *aFileExt, >+nsresult nsOSHelperAppService::GetMIFromExtension(const char *aFileExt, Hey, just spell out MimeInfo for this function and the next one, ok? >Index: uriloader/exthandler/beos/nsOSHelperAppService.h >- NS_IMETHODIMP GetFromMIMEType(const char *aMIMEType, nsIMIMEInfo **_retval); >+ already_AddRefed<nsIMIMEInfo> GetMIMEInfoFromOS(const char *aMIMEType, const char * aFileExt); Weird indent of the new function... Don't you need to decler your GetFromExtension/GetFromMIMEType overrides in the header here? >Index: uriloader/exthandler/mac/nsOSHelperAppService.cpp >+ // If we got two matches, and the type has no default app, copy default app >+ PR_LOG(mLog, PR_LOG_DEBUG, ("OS gave us: By Type: 0x%p By Ext: 0x%p type has default: %s\n", >+ miByType.get(), miByExt.get(), hasDefault ? "true" : "false")); >+ >+ if (!hasDefault && miByType && miByExt) { That comment wants to be before the if, not before the PR_LOG, no? >Index: uriloader/exthandler/os2/nsOSHelperAppService.cpp >+nsOSHelperAppService::GetMIMEInfoFromOS(const char *aType, >+ const char *aFileExt) { >+ nsIMIMEInfo* retval = GetFromType(aType).get(); >+ PRBool hasDefault = PR_FALSE; >+ if (retval) >+ retval->GetHasDefaultHandler(&hasDefault); >+ if (!retval || !hasDefault) { >+ nsCOMPtr<nsIMIMEInfo> miByExt = GetFromExtension(aFileExt).get(); This leaks. Lose the .get(), please. Same thing for Unix. >Index: uriloader/exthandler/win/nsOSHelperAppService.cpp >+static PRBool typeFromExtEquals(const char *aExt, const char *aType) >+ return PR_FALSE; >+ >+} Drop that empty line. >+already_AddRefed<nsIMIMEInfo> nsOSHelperAppService::GetByExt(const char *aFileExt, const char *aTypeHint) GetByExtension, ok? ;) > fileExtToUse.Append(aFileExt); > >- // o.t. try to get an entry from the windows registry. >+ // o.t. try to get an entry from the windows registry. > HKEY hKey; > LONG err = ::RegOpenKeyEx( HKEY_CLASSES_ROOT, fileExtToUse.get(), Is it just me, or is this all mis-indented? If it is, please fix. >+ // OK. We might have the case that |aFileExt| is a valid extension for the >+ // mimetype we were given. In that case, we do want to append that type >+ // to the mimeinfo that we have. You mean "append aFileExt"? Also, don't you only want to append if it's not already in the list? Almost there! ;)

Comment on attachment 128034 [details] [diff] [review] patch v7 r=me.

Comment on attachment 128034 [details] [diff] [review] patch v7 sr=darin

ok. checked in. Bug 213873 filed for webbrowserpersist, bug 213874 filed for nsMessenger, Bug 213875 filed for comment 46 (let the getfrom4xdatabase function return an array of extensions or something like that), Bug 213877 filed for contentAreaUtils.js

I meant to mark this fixed..,

This bug might caused compilation fails on MinGW - bug 215940