521495 - moz-icon:// protocol does not work with SVG icons

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[flying sheep]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 FirePHP/0.3
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.1.3) Gecko/20090824 Firefox/3.5.3 FirePHP/0.3

for the working mime types i tried, the generic orange-gnome-foot-on-sheet-icon was shown, for invalid names, the foot was white.

Reproducible: Always

Steps to Reproduce:
enter url like moz-icon://.pdf?size=16
Actual Results:  
generic icon is shown on kde

Expected Results:  
compare with gnome or windows: pdf icon shows up.

simple pseudocode-patch:

mime = url.substring(firstIndexOf(".")+1, firstIndexOf("?")) + ".png"

size = url.parse("size")
if size <= 16:
  sizestr = "16x16"
else if size <= 24: #kde uses 22² pixel icons, windows 24² for the same purposes
  sizestr = "22x22"
else if size <= 32:
  .
  .
  .
  sizestr = "265x265"

if desktop == "kde":
  while line != "[Icons]"
    line = readline(~/.kde/share/config/kdeglobals)
  theme = readline(~/.kde/share/config/kdeglobals).substring(firstIndexOf("="))
  iconpath = "/usr/share/icons/" + theme + sizestr + "mimetypes" + prefix + mime

i cheated: you have to try out every prefix ("application-","text-",...) and use other sizes if one is not available, but it’s not complicated...

Comment 1

[flying sheep]

my python-script is ready. i reimplemented some moz-icon-parsing and i think that detecting the desktop-enviroment is also already implemented.

just port it, someone, and you are done (oh, and fix one minor TODO)

#! /usr/bin/env python
# -*- coding: utf-8 -*-

from __future__ import print_function
import sys, os
from subprocess import Popen, PIPE

def detectDE():
	if os.environ.get("KDE_FULL_SESSION") == "true":
		return "kde"
	elif os.environ.get("GNOME_DESKTOP_SESSION_ID"):
		return "gnome"
	elif os.environ.get("DISPLAY"):
		return "x"
	return None

url = sys.argv[1] + "?" #the "?" is a bugfix, since string.find() gives back a "-1" when nothing is found.

ext = url[url.find(".") : url.find("?")]

sizeindex = url.find("size=")
if sizeindex == -1: #"size=" not found => no size specified
	size = 16
else:
	sizeend = url.find("?",sizeindex)
	size = int(url[sizeindex+5 : sizeend])

for s in [256,128,64,48,32,22,16]:
	if size <= s:
		sizestr = str(s) + "x" + str(s)

if detectDE() == "kde":
	mime = Popen(["kmimetypefinder", "-f", ext], stdout=PIPE).communicate()[0] #finds mimetype for the extension on kde
	if "\n" in mime: #a two-line-string is only the output if the mimetype is found
		mime = mime[:mime.find("\n")] #remove the accuracy-line
		mime = mime.replace("/","-") # using "-"-mimetypes is more bug-proof
		iconpath = Popen(["kiconfinder", mime], stdout=PIPE).communicate()[0] #finds icon for mimetype
		iconpath = iconpath.replace("\n","") #the line-end is removed
		iconpath = iconpath.replace("48x48", sizestr) #replaces icon size 48×48 with the size specified
	else:
		iconpath = "generic icon path" #to be replaced
	print(iconpath) #the output: you will want to change this to "return iconpath" after making all of this a c++-method ;)

#TODO: Check if requested image size exists and if not, try other sizes

Comment 2

[Tanner Filip [:tanner]]

i see this too. Confirming

Comment 3

[flying sheep]

is only in (k|x)ubuntu karmic koala: https://bugs.launchpad.net/ubuntu/+bug/466567

Comment 5

[Reed Loden [:reed]]

Comment #0 is very hard to follow. Check the launchpad bug or my bug 528382.

Comment 6

[Reed Loden [:reed]]

If this only happens after an upgrade to Ubuntu 9.10, I guess it's not really a regression on Firefox's end.

Comment 7

[Ilja Sekler]

The initial Firefox 3.0 release behaves exactly the same, so likely the default SVG-only icon theme on Ubuntu 9.10 just exposed for the first time some issues or of moz-icon:// protocol.

Actually, it is enough to change the filename extension of SVG icons from .svg to .png (e.g. put symlinks into ~/.icons/Humanity/mimes/$ICON_SIZE/ with .png as extension) to make them "work".

Comment 8

[flying sheep]

remember: this is not only about svg.
i have a kde desktop and use the oxygen icons. every mime icon is a png-picture and the mime icons do not work in firefox.

furthermore i did not uprade, my kubuntu 9.10 is freshly installed!

tu sum it up: the last two comments are misleading, since they only refer to gnome desktops, not kde.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

So this is or isn't a Firefox regression?

re comment 8; flying sheep, I think what Reed is saying is that something in Ubuntu 9.10 may have caused this.

Comment 10

[Ilja Sekler]

(In reply to comment #9)
> So this is or isn't a Firefox regression?

No. But the problem on GNOME seems to live on the Firefox end, so this should be fixed in Firefox. Unsure about the problem on KDE, it is IMHO different and probably a KDE (or even KDE and Ubuntu-specific) bug.

So said, I apologize for the mess I caused asking Reed to dupe his bug 528382 to this one. Don't know whether it would be better to undupe bug 528382 and to give this one a better summary and description or to continue in this bug despite the original intention of the reporter.

Comment 11

[Daniel Veditz [:dveditz]]

Don't see how minor UI polish can possibly be a blocker for security updates.

Comment 12

[Joe Drew (not getting mail)]

OK, let's call this a bug with SVG icons. flying sheep: could you please file a separate bug for your issue, and we will track it down there?

There is absolutely no way this should block.

Comment 13

[flying sheep]

Joe, this is my bug. The Gnome Users just found out that they have a similar issue, too and discussed it here.

Comment 14

[flying sheep]

I made a new Bug report. Could somebody with kde please confirm Bug 532362, since for me, this discussion was a waste of time.

don’t get me wrong, i’m glad you tracked down your .svg-bug here, but initially i reported this and wanted my problem tracked down ;)

Comment 16

[Nils Maier [:nmaier]]

Attachment: v1 central, Fix moz-icon under gnome by using gtk functions

Current patch uses gtk_icon_theme_* which replaces the libgnomeui stuff since some time.
See: http://live.gnome.org/LibgnomeMustDie

A lot of stuff already depends on gtk 2.4+, so depending on it shouldn't be a problem here.

Tested to work under Ubuntu karmic 9.10 x86.

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 418531 [details] [diff] [review]
v1 central, Fix moz-icon under gnome by using gtk functions

Is it possible to add a test on this? Are there any icons that ship with most default gtk themes in SVG format?

Comment 18

[Nils Maier [:nmaier]]

Attachment: v0, xpshell-tests

Some xpcshell tests.

The test will try to open channels for various pre-defined file icons at different sizes.
All tests are expected to succeed.

Some troubles, however:
* On *nix (at least on Ubuntu karmic x86) xpcshell seg faults in exit_handlers after running these tests, no matter if my icon channel patch was applied before or not.
Stack is like this:
#0  0x432dfa40 in ?? ()
#1  0x41758afc in IA__g_hash_table_foreach (hash_table=0x920d720, func=0x42e13460, user_data=0xbfe22984) at /build/buildd/glib2.0-2.22.3/glib/ghash.c:1211
#2  0x42e17333 in ORBit_POA_deactivate () from /usr/lib/libORBit-2.so.0
#3  0x42e17594 in ?? () from /usr/lib/libORBit-2.so.0
#4  0x42e1782d in PortableServer_POA_destroy () from /usr/lib/libORBit-2.so.0
#5  0x42e01c06 in CORBA_ORB_shutdown () from /usr/lib/libORBit-2.so.0
#6  0x42e01d7f in CORBA_ORB_destroy () from /usr/lib/libORBit-2.so.0
#7  0x42e0355f in ?? () from /usr/lib/libORBit-2.so.0
#8  0x4124c05f in __run_exit_handlers (status=153158016, listp=0x4135e304, run_list_atexit=true) at exit.c:78
#9  0x4124c0cf in *__GI_exit (status=0) at exit.c:100
#10 0x41233b5e in __libc_start_main (main=0x804baf6 <main>, argc=18, ubp_av=0xbfe22bc4, init=0x8051c10 <__libc_csu_init>, fini=0x8051c00 <__libc_csu_fini>, rtld_fini=0x4000dd20 <_dl_fini>, 
    stack_end=0xbfe22bbc) at libc-start.c:252
#11 0x0804a541 in _start () at ../sysdeps/i386/elf/start.S:119

Any ideas about that?

* Cannot test on Mac, as I do not own a Mac.

Test work fine under Win32. Win32 already returns an icon, always.

(In reply to comment #17)
> (From update of attachment 418531 [details] [diff] [review])
> Is it possible to add a test on this? Are there any icons that ship with most
> default gtk themes in SVG format?

I don't think you can really make any assumptions about the system/distro in place.
Hence the more general tests.

Comment 19

[Reed Loden [:reed]]

Comment on attachment 418531 [details] [diff] [review]
v1 central, Fix moz-icon under gnome by using gtk functions

>+#if GTK_CHECK_VERSION(2,4,0)

Why do we need such a check here at all? configure.in has GTK2_VERSION=2.10.0 as the minimum version.

Comment 20

[Nils Maier [:nmaier]]

(In reply to comment #19)
> (From update of attachment 418531 [details] [diff] [review])
> >+#if GTK_CHECK_VERSION(2,4,0)
> 
> Why do we need such a check here at all? configure.in has GTK2_VERSION=2.10.0
> as the minimum version.

There are a couple of such checks in already. I didn't know if the configure check is considered enough, so I did stick with the code style already there and added one such check myself.

If these checks are considered "obsolete" I can easily remove the one I added and the others as well.

Comment 21

[Wolfgang Rosenauer [:wolfiR]]

(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 418531 [details] [diff] [review] [details])
> > >+#if GTK_CHECK_VERSION(2,4,0)
> > 
> > Why do we need such a check here at all? configure.in has GTK2_VERSION=2.10.0
> > as the minimum version.
> 
> There are a couple of such checks in already. I didn't know if the configure
> check is considered enough, so I did stick with the code style already there
> and added one such check myself.

IMO the check could make sense still. The reasons for requiring 2.10 are for example the new print dialog. In case someone wants to ship anything based on mozilla and don't need that it's still better to have checks for the other (older) stuff. But that's only my opinion.

Comment 22

[Nils Maier [:nmaier]]

Any idea about the crashing test exit (comment #18)?
If not, maybe implement this as mochitests instead of xpcshell ones? Or what else?

Does this require super-review, and if yes could you point me at the best sreviewer for this type of bug?

Comment 23

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/e63da1dc90b6
http://hg.mozilla.org/mozilla-central/rev/5fda39cd703c

Comment 24

[Kim]

With this fixed, is the "height < 256 && width < 256" checks still really necessary in?:
http://hg.mozilla.org/mozilla-central/file/d0d3ac2ce08c/modules/libpr0n/decoders/icon/gtk/nsIconChannel.cpp#l102

I think the comparator might be wrong too, should I file a new bug for that if the checks should stay in?