Closed Bug 147679 Opened 22 years ago Closed 21 years ago

GetFromMIMEType on Windows only gets one extension and needs rethinking - application/octet-stream always saved as / renamed to EXE with download

Categories

(Core Graveyard :: File Handling, defect, P1)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.6alpha

People

(Reporter: bzbarsky, Assigned: Biesinger)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Hixie-P0],[adt2])

Attachments

(2 files, 9 obsolete files)

1.21 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
148.48 KB, patch
bzbarsky
: review+
darin.moz
: superreview+
Details | Diff | Splinter Review
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.
Blocks: exe-san
Blocks: 129979
Blocks: 142102
Blocks: 161958
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.
Keywords: nsbeta1
Whiteboard: [Hixie-P0]
QA Contact: sairuh → petersen
*** 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.

Assignee: law → bzbarsky
Blocks: 178122
Depends on: 162116
Priority: -- → P1
Target Milestone: --- → mozilla1.3alpha
nsbeta1+/adt2 per the nav triage team.

Keywords: nsbeta1nsbeta1+
Whiteboard: [Hixie-P0] → [Hixie-P0],[adt2]
Blocks: 65827
*** 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...
Blocks: 164188
Blocks: 163882
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.
Target Milestone: mozilla1.3alpha → mozilla1.4beta
Taking the liberty of tweaking the bug summary for better Bugzilla query
discoverability.
Summary: GetFromMIMEType on Windows only gets one extension and needs rethinking → GetFromMIMEType on Windows only gets one extension and needs rethinking - application/octet-stream always saved as / renamed to EXE with download
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.....
Blocks: 167670
Target Milestone: mozilla1.4beta → mozilla1.5alpha
*** Bug 201644 has been marked as a duplicate of this bug. ***
Blocks: 188058
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
Assignee: bzbarsky → cbiesinger
Blocks: 78919
Target Milestone: mozilla1.5alpha → mozilla1.6alpha
Attached patch The IDL part (obsolete) — Splinter Review
here's the .idl change that was missing from the previous patch
Attached patch OS/2 part (obsolete) — Splinter Review
Attachment #127385 - Attachment description: patch → BeOS 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.
Attached patch win patch (obsolete) — Splinter Review
darin: yeah, that's what I meant.

ok... I'll change the callers and remove GetFromType and GetFromExtension
Attachment #127375 - Flags: review?(bzbarsky)
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
Attachment #127375 - Flags: review?(bzbarsky)
Attached patch complete patch (obsolete) — Splinter Review
this patch is missing a line in nsExternalHelperAppService.cpp:
PRLogModuleInfo* nsExternalHelperAppService::mLog = nsnull;

below the headers

please pretend that it's there
Attachment #127375 - Attachment is obsolete: true
Attachment #127376 - Attachment is obsolete: true
Attachment #127382 - Attachment is obsolete: true
Attachment #127385 - Attachment is obsolete: true
Attachment #127389 - Attachment is obsolete: true
Attachment #127400 - Attachment is obsolete: true
Attachment #127808 - Flags: review?(bzbarsky)
Status: NEW → ASSIGNED
Attached patch supplimental patch (obsolete) — Splinter Review
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).
Attachment #127808 - Flags: review?(bzbarsky) → review-
Comment on attachment 127833 [details] [diff] [review]
supplimental patch

This part looks good.  ;)
Attachment #127833 - Flags: review+
>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
Attached patch patch v6 (obsolete) — Splinter Review
bz's comments addressed

(yes... the v6 doesn't match the patches attached here... oh well)
Attachment #127808 - Attachment is obsolete: true
Attachment #127833 - Attachment is obsolete: true
Attachment #127965 - Flags: review?(bzbarsky)
Comment on attachment 127968 [details] [diff] [review]
camino patch

r=me on the camino change.
Attachment #127968 - Flags: review+
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!  ;)
Attachment #127965 - Flags: review?(bzbarsky) → review-
Attached patch patch v7Splinter Review
Attachment #127965 - Attachment is obsolete: true
Attachment #128034 - Flags: review?(bzbarsky)
Comment on attachment 128034 [details] [diff] [review]
patch v7

r=me.
Attachment #128034 - Flags: review?(bzbarsky) → review+
Attachment #128034 - Flags: superreview?(darin)
Blocks: 213516
Comment on attachment 128034 [details] [diff] [review]
patch v7

sr=darin
Attachment #128034 - Flags: superreview?(darin) → superreview+
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..,
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
This bug might caused compilation fails on MinGW - bug 215940
Blocks: 162116
No longer depends on: 162116
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: