229695 - Icon in download dialog is based on extension, not MIME type

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Hixie (not reading bugmail)]

When you download a file and you get the dialog, the icon is based on the
extension of the file, not the MIME type of the file. It should be based on the
MIME type (even if there wasn't one; in those cases we would internally guess
one, not necessarily only on the extension, and that should match the icon).

Comment 1

[Boris Zbarsky [:bzbarsky]]

See
http://lxr.mozilla.org/seamonkey/source/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js#265
and
http://lxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/icon/nsIconURI.cpp#404
and
http://lxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/icon/nsIconURI.cpp#128

I would guess that we need to synthesize a filename for the moz-icon URI with
the "right" extension (based on the MIMEInfo's MIME type, somehow, though we
need to be a little careful because for image/jpeg we should use the "default"
extension for the type, from the MIME info, whereas for application/octet-stream
we should not use an extension from the MIME info).

Perhaps the logic should be:

1)  Get type for default extension on mime info
2)  If that matches the type on mime info, use that default extension
3)  If not, get default extension for the mime info's type
4)  Synthesize a filename with that extension and use it.

This is assuming we want the icon to match the type; I have no preference either
way on that question.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

the relevant code is at
http://lxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/icon/win/nsIconChannel.cpp#249

I don't think it would be a good idea to change nsIconURI, why can't the icon
channels decide that for themselves?

Comment 3

[Hixie (not reading bugmail)]

Creating a fake URI assumes that all implementations of the icon service use
extensions for working out the icon. This might not always be true. Couldn't we
just pass the MIME type? "moz-icon:?mime=text/plain" or something. We know what
the MIME type is by that point.

Comment 4

[[e-mail]]

We already do, but the windows icon channel prefers to use the extension...

Comment 5

[Hixie (not reading bugmail)]

Sounds like a bug in the Windows icon channel then, rather than a bug in the way
it is called. :-)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ian, this is not the only consumer of icon URIs.

Perhaps we just need to clearly respec what icon URIs _should_ do.  Then either
fix the helper app dialog or the impls as needed.

Comment 7

[Hixie (not reading bugmail)]

Sure -- my point was just that as I see it, _if_ a MIME type is specified on the
moz-icon: URI, then that should take priority over the extension. If one is not
specified, then the extension should be used. Why would that break other call
sites? (And why would you want to specify the MIME type on the URI if it isn't
used? I'm having difficulty understanding why this is controversial.)

Comment 8

[Boris Zbarsky [:bzbarsky]]

It's not controversial.  It just needs someone to look over all callsites to
make sure nothing breaks.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, I basically have a patch...

Hixie: Consider the case that windows (this patch is windows specific. this bug
actually does not occur on linux, where we do not show any icon :) ) knows
nothing about a specific mime type, consequently does not have an icon for it.

Should we:
a) Show no icon
b) Show the icon matching the file extension
c) Show some generic icon
d) do something else

Comment 10

[Hixie (not reading bugmail)]

That is a good question.

The thing to avoid, IMHO, is us saying:

   File type: Unknown type (application/x-foo-bar)
   File icon: [the HTML file icon]

...because then we look silly.

Maybe we could show a generic icon based on the top-level MIME type?

e.g.:

   unknown text/* - show text/plain icon
   unknown application/* - show application/octet-stream icon
   unknown image/* - show an image icon
   unknown audio/* - show a sound icon

...etc.

If that's too hard, though, I'm not strongly against just showing the
extension's icon. After all, we don't know _what_ icon is relevant, and at that
point the icon is just prettiness, right? And presumably using the extension's
icon would match the icon of the default application, too.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

ok, summary of the irc discussion between hixie and me:

o) The icon we show is the icon of the file (i.e. of the mime type), not
necessarily the application we open it with
o) Hence, if we can't show an icon matching the mime type, we should show the
generic icon

I'll attach a patch for that.

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v2

this patch now only uses the content type if the file is not local. for local
files, it just calls SHGetFileInfo on the file path and gets the real icon for
the file.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 138312 [details] [diff] [review]
patch v2

Sure, looks OK.  sr=bzbarsky.  Does MacOS need similar changes?

Comment 15

[Christian :Biesinger (don't email me, ping me on IRC)]

macos doesn't seem to need them...
http://lxr.mozilla.org/mozilla/source/modules/libpr0n/decoders/icon/mac/nsIconChannel.cpp#325

however, OS/2 probably does. I have no idea if asking the OS for an icon for "."
works there though... cc'ing mkaply

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

patch checked in. leaving bug open for os/2

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

I changed my mind and filed Bug 235018 about os/2.

this bug is fixed.