714068 - firefox icon no longer displayed in gnome-panel

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect)

Description

[nulldomain]

Attachment: Screenshot.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:12.0a1) Gecko/20100101 Firefox/12.0a1
Build ID: 20111229013125

Steps to reproduce:

Since sometime in November or early December Firefox no longer shows the site favicon in gnome-panel.  Screenshot attached.

Comment 1

[nulldomain]

Closing this bug as it turns out this is an intended feature after bug 562506 landed. When building and installing from source firefox needs to install it's icons in /usr/share/icons/hicolor/sizexsize/apps so the right program icon is picked up by the theme.  Guess I need to open a new bug for that.

Comment 2

[Jean-Alexandre Anglès d'Auriac]

No, it is not the intended feature. Firefox should still ship with default icons that should be loaded even if the icon theme does not provide a Firefox icon.
How was the Firefox used in the example obtained ? Is it from an official build ?

Comment 3

[nulldomain]

I regularly build firefox from trunk do make install to install.  The only icons I see installed are in /usr/lib/firefox-12.0a1/chrome/icons/default.  As a work around I copied the icons to /usr/share/icons/hicolor/ to the appropiate size and apps folder, and renamed them firefox.png to get the icon back in the panel.

Comment 4

[Jean-Alexandre Anglès d'Auriac]

My patch was supposed to check if the current icon theme provide icons for Firefox, and load those icons in /usr/lib/firefox-12.0a1/chrome/icons/default if the icon theme does not !
What is the name of the icons in this directory ? Is it default16.png, default32.png and default48.png ? I should maybe compile Firefox 12, to try to reproduce this bug. I only tested the patch on 11.0a1 (2011-12-12).

Comment 5

[Karl Tomlinson (:karlt)]

I wonder whether there may be some situation where gtk_icon_theme_has_icon returns TRUE but GtkWindow does not use it (for gtk_window_set_icon_name).

icon_list_from_theme() in gtkwindow.c uses gtk_icon_theme_get_icon_sizes() and gtk_icon_theme_load_icon().

In bug 562506 comment 43, I suggested using gtk_icon_theme_get_icon_sizes() to check.  I can't remember why I said that rather than gtk_icon_theme_has_icon.

Then in bug 562506 comment 52, I thought that gtk_window_get_icon() after calling gtk_window_set_icon_name() was better.

Comment 6

[Karl Tomlinson (:karlt)]

I don't know how prevalent this is, but we may need to fix this for 11.

Comment 7

[nulldomain]

Yes the names of the icons in /usr/lib/firefox-12.0a1/chrome/icons/default are default16.png, default32.png, and default48.png.

Comment 8

[Jean-Alexandre Anglès d'Auriac]

Ok.
Well, there are two different way it could fail : either it is gtk_icon_theme_has_icon that returns TRUE when the expected result is FALSE, or there is a problem in the routine that is supposed to add builtin icons.
I'm hg pull-ing and I'll try to build the see if I can reproduce the bug, but that takes a crazily long time on my machine. In the meantime, Joseph, could you use the small test program I wrote in bug 562506 comment 111 ? Thanks.

Comment 9

[Jean-Alexandre Anglès d'Auriac]

I finally managed to compile firefox, but I couldn't reproduce the bug.

Comment 10

[nulldomain]

I compiled the program and ran it.  It says the Default theme has an firefox icon but it doesn't show up in the window.  It only shows up in the window if I copy the firefox icon into hicolor/sizexsize/apps folder.

Comment 11

[Karl Tomlinson (:karlt)]

It seems that gtk_icon_theme_has_icon includes unthemed icons, but GtkWindow does not.  I can reproduce using Josephs program by (re)moving themed icons and icon-theme.cache, because there is a /usr/share/pixmaps/firefox.png.  It should be possible to reproduce by adding a firefox icon to the top level directory of any of the icon paths. e.g. ~/.icons/firefox.png

Comment 12

[Jean-Alexandre Anglès d'Auriac]

Thanks Karl Tomlinson. I can now reproduce the bug too, event though I don't really understand why gtk_icon_theme_has_icon has this behavior.
So, I see two solutions :
- we can make a more extensive test to check if the theme actually provides the icon or not
- we can use the other patch I provided, the one that add the default icons as builtin once

The cost of the first method is that every time SetDefaultIcon is called, we must do a possibly long check to see if the icon is actually provided by the theme, the advantage is that we don't need to add builtin when they are not needed.
The cost of the second method is that we always need to add builtins, but the advantage is that we have a maybe shorter checking time needed everytime SetDefaultIcon is called (checking for an icon in the empty theme cost should cost much less than checking for an icon theme including many folders).
I tend to think the second method is better, but I can create a patch that implement the first one if you think measurement and comparison is needed.

Comment 13

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #11)
> I can reproduce using Josephs program

Sorry that was Jean-Alexandre's program.  Very helpful, thanks.

(In reply to Karl Tomlinson (:karlt) from comment #5)
> Then in bug 562506 comment 52, I thought that gtk_window_get_icon() after
> calling gtk_window_set_icon_name() was better.

But that won't work until the window is realized, so that seems less than ideal.

(In reply to Jean-Alexandre Anglès d'Auriac from comment #12)
> So, I see two solutions :
> - we can make a more extensive test to check if the theme actually provides
> the icon or not

gtk_icon_theme_get_icon_sizes() would work there.

> The cost of the first method is that every time SetDefaultIcon is called, we
> must do a possibly long check to see if the icon is actually provided by the
> theme, the advantage is that we don't need to add builtin when they are not
> needed.

We benefit from GTK's caching, and so this should not be costly.

> The cost of the second method is that we always need to add builtins, but
> the advantage is that we have a maybe shorter checking time needed everytime
> SetDefaultIcon is called (checking for an icon in the empty theme cost
> should cost much less than checking for an icon theme including many
> folders).

On most systems, given the extra files loaded, I don't think this will be faster.

Having to choose how we test based on our knowledge of GtkWindow internals is unfortunate, but switching from _has_icon() to _get_icon_sizes() should be a simple change.

Comment 14

[Jean-Alexandre Anglès d'Auriac]

Attachment: replace gtk_icon_theme_has_icon by gtk_icon_theme_get_icon_sizes

Should we make a bug report to GTK about that strange behavior, or not ?

Comment 15

[Alex Keybl [:akeybl]]

Now tracking for resolution during the Aurora 11 time period. We'd consider backing out bug 562506 instead if we're not comfortable in our understanding by Beta 10.

Comment 16

[Karl Tomlinson (:karlt)]

Comment on attachment 587065 [details] [diff] [review]
replace gtk_icon_theme_has_icon by gtk_icon_theme_get_icon_sizes

gtk_icon_theme_get_icon_sizes returns a zero-terminated array.  It never returns NULL.  We need to check whether the first entry of that array is non-zero, and free the array with g_free.

(In reply to Jean-Alexandre Anglès d'Auriac from comment #14)
> Should we make a bug report to GTK about that strange behavior, or not ?

Sounds good if you are willing, but I expect it's unlikely to be changed unless this is a recent regression or you feel like submitting a patch.
Bear in mind that GTK development is on GTK3 now, so check the bug still exists there before filing.

Comment 17

[Jean-Alexandre Anglès d'Auriac]

Attachment: replace gtk_icon_theme_has_icon by gtk_icon_theme_get_icon_sizes

The bug is still present in gtk3. I plan to make a bug report anyway, if it was not already done.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 587132 [details] [diff] [review]
replace gtk_icon_theme_has_icon by gtk_icon_theme_get_icon_sizes

>-    bool foundIcon = gtk_icon_theme_has_icon(gtk_icon_theme_get_default(),
>-                                             iconName.get());
>-
>-    if (!foundIcon) {
>+    bool foundIcon = false;
>+    gint * iconsSize = gtk_icon_theme_get_icon_sizes(gtk_icon_theme_get_default(),iconName.get());
>+
>+    if (iconsSize[0]) {

This test is reversed, but the logic here can be localized by initializing foundIcon to

   bool foundIcon = iconSizes[0] != 0;

Then iconSizes can be freed immediately, the "if (!foundIcon)" can remain, and the else block is unnecessary.

Please follow file style, including spaces after commas in parameter lists, staying within 80 columns, and removing one of the spaces in the "gint * iconSizes" (file style is not consistent on which space to leave, but having both spaces looks like multiplication).

Comment 19

[Jean-Alexandre Anglès d'Auriac]

Attachment: replace gtk_icon_theme_has_icon by gtk_icon_theme_get_icon_sizes

Comment 20

[Karl Tomlinson (:karlt)]

I changed variable names in attachment 587178 [details] [diff] [review] to make it compile, tested, and pushed https://hg.mozilla.org/integration/mozilla-inbound/rev/6d755ac01cbb

Comment 21

[nulldomain]

I tested the patch and it works for me.

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6d755ac01cbb

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I confirm that this landing fixed the problem I was seeing (bug 562506 comment 107) as well.

Comment 24

[Alex Keybl [:akeybl]]

I think this landed without an r+..

Regardless, please nominate for Aurora 11 with a risk/reward comparison versus backing out 562506 and waiting till FF12 to use the system icon.

Comment 25

[Karl Tomlinson (:karlt)]

Attachment: changeset for aurora approval request

[Approval Request Comment]
Regression caused by (bug #): 562506
User impact if declined: Firefox window icon replaced with fallback generic icon on some systems.
Testing completed (on m-c, etc.): on m-c.  3 users have verified that it fixes the bug.
Risk to taking this patch (and alternatives if risky):
Risk is very low because the patch is merely changing the function called to test for presence of a themed icon to the same function that GTK itself uses.
Using the same function that GTK itself uses gives assurance against the risk of unexpected side effects from any bugs in the OS function.

(In reply to Alex Keybl [:akeybl] from comment #24)
> Regardless, please nominate for Aurora 11 with a risk/reward comparison
> versus backing out 562506 and waiting till FF12 to use the system icon.

The risk of backing out is also very low as I'm not aware of any other recent changes that interact with this code.
A reward of not backing out is that icon files are opened and read less during opening of new windows, including during start-up, which should provide performance benefits.  (I don't have a measurement - timing would be dependent on a number of system factors.)

Comment 26

[Alex Keybl [:akeybl]]

Comment on attachment 588614 [details]
changeset for aurora approval request

[Triage Comment]
Low risk patch, pretty good reward. We're expecting to see fallout from this quickly if it causes any regressions.

Comment 27

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3c403469f834

Comment 28

[Maniac Vlad Florin (:vladmaniac)]

Build identifier: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20100101 Firefox/11.0

Icon is properly displayed in gnome. Verified for Fx 11 Beta 2