397823 - moz-icon URL for nonexistent content type doesn't return generic icon

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

A moz-icon URL for a nonexistent content type returns a generic icon on Windows and Linux, but it doesn't load on Mac.  For example:

moz-icon://goat?size=16&contentType=nonexistent/type
moz-icon://goat?size=16

Returning a generic icon for such types is useful, because it allows code that uses moz-icon URLs to display generic icons in place of type-specific icons without extra code to determine whether or not a type has an icon on the given platform.

Like on Windows and Linux, moz-icon on the Mac should return a generic icon for moz-icon URLs with nonexistent content types.

Comment 1

[Jesse Ruderman]

There's a similar problem for the file called "README" in ftp://ftp.mozilla.org/.  It appears to be due to

  moz-icon://unknown?size=16

not returning the generic icon like

  moz-icon://.zzz?size=16

does.

Comment 2

[Jesse Ruderman]

Attachment: screenshot of ftp://ftp.mozilla.org/

README has the text "File:" next to it instead of an icon.  This is confusing.

Comment 4

[Seth Spitzer]

On a possibly related note, in 1.9.1, the following worked for me:

moz-icon://goat?size=16&contentType=text/html
moz-icon://goat?size=16&contentType=application/pdf

We would show the icon (heeding the contentType).

In 1.9.2, neither of those urls work, similar to myk's reported issue.

I have not tried this on Windows yet, only mac.

FWIW, I believe Firefox still uses this "goat" trick, see http://mxr.mozilla.org/mozilla1.9.2/search?string=moz-icon://goat

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Possible Fix

This fixes it for me on 1.9.2.  

However, I don't know

1) if this code compiles for 64-bit for m-c
2) if there are any unintended consequences of always returning the kGenericDocumentIcon here
3) why this used to work on 1.9.1, since this code is as "broken" in 1.9.2/m-c as it was in 1.9.0 when I got duped here

What's happening in this bug is that for goats (or similar) a) we don't have a localFile, b) we don't have an extension (unlike .zzz in Jesse's working example), so we just NS_ERROR_FAILURE at http://mxr.mozilla.org/mozilla1.9.2/source/modules/libpr0n/decoders/icon/mac/nsIconChannelCocoa.mm#279 (and then, in debug builds, log NS_ENSURE_SUCCESS failures at http://mxr.mozilla.org/mozilla1.9.2/source/modules/libpr0n/decoders/icon/mac/nsIconChannelCocoa.mm#220 when trying to load moz-icon://unknown? or moz-icon://goats? and so forth).

I'll toss this to the try-server for m-c tomorrow and see if I can find someone with a 64-bit build to make sure the HFSTypeCode constants are still around there, and then request review unless someone has info about 2 or 3.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #5)
> 3) why this used to work on 1.9.1, since this code is as "broken" in 1.9.2/m-c
> as it was in 1.9.0 when I got duped here

Oh, this used to work for Seth because he used a known content-type, and on 1.9.0 and 1.9.1 known content-types went through the MIME service first (which got ripped out by the killing of nsIInternetConfigService in bug 489864).  Unknowns never worked.

So, my patch doesn't really fix Seth's bug (which is a separate regression from bug 489864), but it does fix this bug; Seth just gets a generic icon now instead of an abject failure, but not the appropriate icon for the content-type as in the past.  There might be a UTI-based API that we could use to replace nsIInternetConfigService for the purpose of matching just content-types to icons; dunno.  Seth, you probably want a separate bug on that regression.

Comment 7

[philippe (part-time)]

(In reply to comment #5)

> 1) if this code compiles for 64-bit for m-c

Tested a 64-bit / 10.6 SDK build: build succeeded.

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Seth, one more thing:

(In reply to comment #6)
> There might be a UTI-based API that we could use to replace
> nsIInternetConfigService for the purpose of matching just content-types to
> icons; dunno.

According to the comments in bug 489864 (e.g. bug 489864 comment 7 and bug 489864 comment 12), there's replacement code in the patch there for getting extension<->MIME type mappings.

So you should be able to fix your bug by getting the MIME service, asking it for the primary extension for the MIME type, turning that into an NSString, and then
 iconImage = [[NSWorkspace sharedWorkspace] iconForFileType:fileExtension];
(basically restoring the code that bug 489864 ripped out, except keying the icon off the extension instead of the HFS code; personally, I'd add it back after the file extension check rather than where it was, given that an icon-for-file-extension is going to be a "known" whereas icon-for-MIME-type-via-extension-for-MIME-type is more of a guessing game).  

I tried to do it myself, but I don't have enough (any!) C++/XPCOM fu to whisper the magic incantations to get me the default extension for the MIME type.

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #8)
> (basically restoring the code that bug 489864 ripped out

…of nsIconChannelCocoa.mm (not the whole nsIInternetConfigService patch, in case that isn't clear).

Comment 10

[Seth Spitzer]

Smokey, thanks for following up on this!

> So you should be able to fix your bug by getting the MIME service, asking it
> for the primary extension for the MIME type

That's exactly what I've done in my code and I can verify it works.

If I don't know the extension, I get it from the mime service like this:

var mimeService = Cc["@mozilla.org/uriloader/external-helper-app-service;1"].getService(Ci.nsIMIMEService); 
var handlerInfo = mimeService.getFromTypeAndExtension(aContentType, null); 
var fileExt = "." + handlerInfo.primaryExtension; 

And then I use that to construct the moz-icon url.

So before, if my file name was "goat" (no extension), I would have: 

moz-icon://goat?size=16&contentType=application/pdf

But now, I'll now have:

moz-icon://.pdf?size=16&contentType=application/pdf

And everything works like a charm.

> Seth, you probably want a separate bug on that regression.

I think we still want a bug on "moz-icon://goat?size=16&contentType=application/pdf" regressing and not heeding the contentType (if there is no extension).  If you agree, I can log it.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #10)
> Smokey, thanks for following up on this!
> 
> > So you should be able to fix your bug by getting the MIME service, asking it
> > for the primary extension for the MIME type
> 
[...]
> > Seth, you probably want a separate bug on that regression.
> 
> I think we still want a bug on
> "moz-icon://goat?size=16&contentType=application/pdf" regressing and not
> heeding the contentType (if there is no extension).  If you agree, I can log
> it.

Yes, indeed. "moz-icon://goat?size=16&contentType=application/pdf" regressing is what I was referring to every time I used "your bug"/"that regression"; sorry I wasn't clear.  nsIconChannelCocoa absolutely should fix itself so that it does the "find the extension for the content-type" and you don't have to do the gymnastics on your end (and restore platform parity); Steven was concerned that the changes in bug 489864 would break subtle things in the platform that wouldn't be obvious, and you found one such thing :D  Feel free to CC me when you file it.

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fix, v1.1

Josh, you've reviewed every Mac-specific change to this file, and it's Cocoa code, so I assume you're the right reviewer even though the file lives in libpr0n.

The only change since the first version of the patch is that I fixed the punctuation and whitespace in my comment.

Comment 13

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: v1.2, with NSFileTypeUnknown instead

Josh, the NSFileTypeUnknown constant does work; here's the patch with that instead of the other gymnastics.

Comment 14

[Seth Spitzer]

> > I think we still want a bug on
> > "moz-icon://goat?size=16&contentType=application/pdf" regressing
> > If you agree, I can log it.
> Yes, indeed.

I've logged bug #558552 on this issue.

Smokey: thanks for all the help!

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/mozilla-central/rev/9acb882b2890

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 437933 [details] [diff] [review]
v1.2, with NSFileTypeUnknown instead

This is a simple patch that fixes a platform-parity bug (noticeable in dirListings and probably elsewhere in chrome), and it also makes bug 558552 (a regression new to 1.9.2) less bad, by showing the generic icon instead of not showing anything at all.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 437933 [details] [diff] [review]
v1.2, with NSFileTypeUnknown instead

This flag got orphaned by the milestone juggling; moving to 1.9.2.7.  See comment 16 for the original request comments.

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 437933 [details] [diff] [review]
v1.2, with NSFileTypeUnknown instead

Approved for 1.9.2.9, a=dveditz for release-drivers

Comment 19

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/8d910462ad83

Comment 20

[passfree]

I don't know what is the status of this bug but sure the following don't work on the latest FF (10) on mac:

moz-icon://goat?size=16&contentType=image/gif
moz-icon://goat?size=16&contentType=application/pdf
moz-icon://goat?size=16&contentType=application/javascript

You just see a blank icon.