562506 - Process icon are builtin, while launcher icon uses the system icon theme

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Jean-Alexandre Anglès d'Auriac]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100402 Lightningphoenix/3.6.3 (Namoroka/3.6.3 polymorphe)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.3) Gecko/20100402 Lightningphoenix/3.6.3 (Namoroka/3.6.3 polymorphe)

The process icons are taken from the /usr/lib/firefox-3.6/chrome/icons/default directory, while the launcher icon is taken from the system icon theme, which is chosen by the user, and might provide a custom icon for Firefox.

This can create a lot visual inconsistencies, especially when using a dock that will use the launcher icon.
See screenshot for more details :
http://i12.photobucket.com/albums/a243/OxayotlTheGreat/Capture-1-16.png

Reproducible: Always

Actual Results:  
http://i12.photobucket.com/albums/a243/OxayotlTheGreat/Capture-1-16.png

Expected Results:  
The process icon should be taken from the system icon theme.

Comment 1

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that fix the bug, but is quite ugly

Comment 2

[Pascal Chevrel:pascalc]

not major

Comment 3

[yannubuntu]

FYI : http://www.omgubuntu.co.uk/2010/09/cmon-mozilla-show-me-my-firefox-icon.html

Comment 4

[Jean-Alexandre Anglès d'Auriac]

I forgot to mention that the patch is for the file widget/src/gtk2/nsWindows.cpp

Comment 5

[Pascal Chevrel:pascalc]

I am going to mark as New since Yi mentioned in forums that he is seeing the bug with our recompiled firefox code on archlinux and not with a distro version of Firefox.

Yi, here is the documentation to make a patch on mozilla code that can be reviewed:
https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch

CCing Paul Rouget, from the engagement team since this bug derives from a thread in French Ubuntu Forums on how to get into our bug reporting process (http://forum.ubuntu-fr.org/viewtopic.php?pid=3702792#p3702792)

Comment 6

[Jean-Alexandre Anglès d'Auriac]

Attachment: Same as the first patch, but built the canonical way

Comment 7

[Pascal Chevrel:pascalc]

(Yi had problems with bugzilla, I asked the review to roc for him)

Comment 8

[Zibi Braniecki [:zbraniecki][:gandalf]]

Comment on attachment 471473 [details] [diff] [review]
Same as the first patch, but built the canonical way

Hi Yi, thanks for your contribution!

>+    list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",16,GTK_ICON_LOOKUP_FORCE_SIZE,NULL));

We don't put an extra white space after function name

>+    list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",32,GTK_ICON_LOOKUP_FORCE_SIZE,NULL));
>+    list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",48,GTK_ICON_LOOKUP_FORCE_SIZE,NULL));
>+    list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",64,GTK_ICON_LOOKUP_FORCE_SIZE,NULL));
>+    list = g_list_append(list, gtk_icon_theme_load_icon (gtk_icon_theme_get_default (),"firefox",128,GTK_ICON_LOOKUP_FORCE_SIZE,NULL));


You could put get_icon_theme_get_default out of the cycle as:

 GtkIconTheme *theme = gtk_icon_theme_get_default();

Comment 9

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

Comment 10

[Jean-Alexandre Anglès d'Auriac]

Thanks, Zbigniew. I updated the patch.

Comment 11

[Zibi Braniecki [:zbraniecki][:gandalf]]

Great, sorry I didn't catch this earlier, but as you're waiting for roc to give a review, another nit - add spaces after each arg., like:

g_list_append(list, gtk_icon_theme_load_icon(theme, "firefox", 128, GTK_ICON_LOOKUP_FORCE_SIZE, NULL));

Also, I'm not sure if "firefox" should be hardcoded here (we have the brandShortName, which is different for Minefield, Firefox, Seamonkey etc.)

Comment 12

[Jean-Alexandre Anglès d'Auriac]

Well, I don't know if minefield's .desktop use a specific icon. But if it does, then you're right. If it doesn't, we need another way to adapt it.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

With this patch, we ignore aIconList. Perhaps we should be asking the theme first and then, if the theme didn't give us anything, use aIconList with the existing code?

Gandalf is right that we shouldn't hardcode "firefox" here. That would definitely break Thunderbird for example. You could get the brandShortName like we do here for example:
http://mxr.mozilla.org/mozilla-central/source/browser/components/shell/src/nsGNOMEShellService.cpp#242

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Also instead of hand-writing several calls to gtk_icon_theme_load_icon to build this list, why can't we just call gtk_window_set_icon_name? We could call gtk_icon_theme_has_icon first, if the theme has an icon then call gtk_window_set_icon_name, otherwise take the existing code path.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, there's also the question of whether it's right to give all windows the same icons. Don't some XUL apps have different icons for different windows of the same application?

Comment 16

[Zibi Braniecki [:zbraniecki][:gandalf]]

(In reply to comment #15)
> Don't some XUL apps have different icons for different windows of
> the same application?

Neither Firefox not Thunderbird does that, but maybe Seamonkey? CC'ing Kairo.

Comment 17

[Robert Kaiser]

SeaMonkey has different icons for different windows, yes, and we really should keep it that way. We're even working on having different kinds of windows grouped differently on Win7 (the platform supports that!), would be a shame if other platforms would go in the contrary way...

Comment 18

[Zibi Braniecki [:zbraniecki][:gandalf]]

sure. So roc's suggestion from comment 13 makes all the sense in the world, right?

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> sure. So roc's suggestion from comment 13 makes all the sense in the world,
> right?

That's something someone with code knowledge needs to reply to, I have no idea what e.g. aIconList is, nor should I need to care, right?

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If we just use my suggestion in comment #13, that will break apps like Seamonkey that want different icons for different windows.

To fix this properly, SetWindowIconList needs more information. We need a string we can use to look up the icon theme. nsWindow::SetIcon gets a string, though. Can we move the code to look up the icon them into SetIcon and extract a string to use from the parameter?

Comment 21

[Jean-Alexandre Anglès d'Auriac]

Ok, so, just to check if I understand right how it works : all the code in nsWindows.cpp is shared between all XUL applications. The functions that are called by application-specific code are nsWindow::SetIcon and nsWindow::SetDefaultIcon .
So I think we should change nsWindow::SetDefaultIcon in such a way that it looks in the current GTK icon theme for an icon called like brandShortName. If it does found one, it calls SetWindowIconList on them. To build the path list we must provide to SetWindowIconList, we can use the gtk_icon_info_get_filename. If it does not found one, we revert to the current behaviour.
How does that sounds to you ? Should I try to write a patch that works this way ?

Comment 22

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that fix the bug, but is quite ugly

One good improvement would be to make the string completely lowercase.

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This patch is pretty good. I don't think you need those new #includes, though.

Comment 24

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

You were right, the first include was a leftover of my first attempt to get the brandname, and the second was here only for debugging purpose, when I added a printf…
Now the brandname is converted to lowercase, because gtk_icon_theme_load_icon is case-sensitive, and iconnames are always lowercase-only.

Comment 25

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

I forgot to free the string I used in my previous patch. And cBrand.get() returns a null-terminated string, so I don't need to use strlen.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 478216 [details] [diff] [review]
Improved patch

Thanks!

Comment 27

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

Added a fallback mechanism that uses the old way to get the icon if it is not present in the GTK theme.
Useful for some brandname that are not included in most GTK theme, like minefield.

Comment 28

[Benoit]

(In reply to comment #27)
> Created attachment 478755 [details] [diff] [review]
> Improved patch

I think you actually meant to ask for a new review, instead of reviewing the patch yourself?

You should probably edit your attachment to set the review field to "?" and put roc as requestee again.

Comment 29

[Jean-Alexandre Anglès d'Auriac]

Yes, you are right. Thanks.

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We use camelCase. So use currentIcon and iconName.

+    if(currenticon)
+    {list = g_list_append(list, currenticon);}

Our style requires
  if (currentIcon) {
    list = g_list_append(list, currentIcon);
  }

Also, instead of repeating the same code 5 times, use a loop. E.g.
  static const int sizes[5] = { 16, 32, 48, 64, 128 };
  for (int i = 0; i < NS_ARRAY_LENGTH(sizes); ++i) {

Comment 31

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

Thanks !
I also camelCased iconname to iconName.

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+        currentIcon = gtk_icon_theme_load_icon(theme,iconName,16,GTK_ICON_LOOKUP_FORCE_SIZE,NULL);

You shouldn't be using "16" here

+        if (currentIcon) {
+        list = g_list_append(list, currentIcon);
+        }

Fix indent

+    if (!list)
+    {

{ on the same line as the if

Comment 33

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

Fixed.

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, the code looks correct now, but there are just a few more style nits :-)

+    gchar* iconName = g_utf8_strdown(cBrand.get(),-1);

space after ,

+        currentIcon = gtk_icon_theme_load_icon(theme,iconName,sizes[i],GTK_ICON_LOOKUP_FORCE_SIZE,NULL);

space after commas

+    if (!list){

Space between ) and {

Comment 35

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

Done.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 481973 [details] [diff] [review]
Improved patch

+        currentIcon = gtk_icon_theme_load_icon(theme, iconName,sizes[i], GTK_ICON_LOOKUP_FORCE_SIZE, NULL);

You missed a space here. Please fix that, attach the final patch and request checkin-needed. Thanks!!!

Comment 37

[Jean-Alexandre Anglès d'Auriac]

Attachment: Improved patch

Comment 38

[Jean-Alexandre Anglès d'Auriac]

Err, I had already put checkin-needed in the keyword of the bug. Is it the right way ?

Comment 39

[Mounir Lamouri (:mounir)]

Comment on attachment 482018 [details] [diff] [review]
Improved patch

Roc, could you approve the patch? It looks like it can't land now...

Comment 40

[Mounir Lamouri (:mounir)]

Attachment: Update configure.in

Unfortunately, GTK_ICON_LOOKUP_FORCE_SIZE needs gtk2-2.14.0, see:
http://library.gnome.org/devel/gtk/unstable/GtkIconTheme.html#GTK-ICON-LOOKUP-FORCE-SIZE:CAPS

So we need to update configure.in but also our build machines (this seems to be the hard part)

Comment 41

[Mounir Lamouri (:mounir)]

Attachment: Refreshed patch

Jean-Alexandre, I refreshed your patch while trying to push it (and fixed a warning).

Comment 42

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 482486 [details] [diff] [review]
Update configure.in

This is fine by me, but I'd prefer that the gtk2 widget people approved this.

Comment 43

[Karl Tomlinson (:karlt)]

Comment on attachment 482487 [details] [diff] [review]
Refreshed patch

>+        currentIcon = gtk_icon_theme_load_icon(theme, iconName, sizes[i], GTK_ICON_LOOKUP_FORCE_SIZE, NULL);

GTK_ICON_LOOKUP_FORCE_SIZE doesn't seem right here

  "gtk_window_set_icon_list() allows you to pass in the same icon in
   several hand-drawn sizes. The list should contain the natural sizes
   your icon is available in; that is, don't scale the image before
   passing it to GTK+. Scaling is postponed until the last minute,
   when the desired final size is known, to allow best quality."

and GtkWindow does not use GTK_ICON_LOOKUP_FORCE_SIZE.

AFAIK there is no theme-change handling here (unless SetDefault is going to be
called every time the theme changes).

I think it would be best to let GTK do the lookup, set the icon list, and
handle theme changes.
This can be done by using gtk_window_set_icon_name().

The slightly tricky bit then is determining whether GTK will find icons for
that name.  It looks like it should be reasonable to assume that, if
gtk_icon_theme_get_icon_sizes() returns a non-empty array, then GTK will have
some icons to use.

>+    gtk_window_set_icon_list(GTK_WINDOW(mShell), list);

This function will need an early return when mShell is NULL.

Comment 44

[Karl Tomlinson (:karlt)]

Comment on attachment 482486 [details] [diff] [review]
Update configure.in

>-GTK2_VERSION=2.10.0
>+GTK2_VERSION=2.14.0

It is about time we updated out minimum requirements but that should follow broader discussion in newsgroups and will require updating build machines.

GTK_ICON_LOOKUP_FORCE_SIZE is not a good reason to make this change, though.

Comment 45

[Mounir Lamouri (:mounir)]

Jean-Alexandre, is it possible to use something else than GTK_ICON_LOOKUP_FORCE_SIZE which is available in 2.10 and open a follow-up to use GTK_ICON_LOOKUP_FORCE_SIZE when gtk-2.14.0 will be required?

Comment 46

[Jean-Alexandre Anglès d'Auriac]

I think we can just remove the GTK_ICON_LOOKUP_FORCE_SIZE. But I am completely unable to figure out how to call gtk_icon_theme_load_icon without a GtkIconLookupFlags argument…
That's actually why I put GTK_ICON_LOOKUP_FORCE_SIZE in the first place.

Comment 47

[Mounir Lamouri (:mounir)]

(In reply to comment #46)
> I think we can just remove the GTK_ICON_LOOKUP_FORCE_SIZE. But I am completely
> unable to figure out how to call gtk_icon_theme_load_icon without a
> GtkIconLookupFlags argument…
> That's actually why I put GTK_ICON_LOOKUP_FORCE_SIZE in the first place.

Would another flag from this list working?
http://library.gnome.org/devel/gtk/unstable/GtkIconTheme.html#GtkIconLookupFlags

Otherwise, you can try to pass 0 and test if that's working as expected?

Comment 48

[Mounir Lamouri (:mounir)]

(In reply to comment #47)
> Otherwise, you can try to pass 0 and test if that's working as expected?

Actually, GTK_ICON_LOOKUP_NO_SVG is equal to 0... I guess you might know (or at least test) which flag should be used.

Comment 49

[Jean-Alexandre Anglès d'Auriac]

We could try GTK_ICON_LOOKUP_GENERIC_FALLBACK. I've already tried passing 0, it won't compile. I also tried NULL, or just removing the argument…

Comment 50

[Mounir Lamouri (:mounir)]

(In reply to comment #49)
> We could try GTK_ICON_LOOKUP_GENERIC_FALLBACK. I've already tried passing 0, it
> won't compile. I also tried NULL, or just removing the argument…
Passing 0 isn't valid because it's expecting a GtkIconLookupFlags type. NULL is often equivalent to 0 is C++ (might be (void*)0 sometimes). Removing the argument can't work because, in C++, the argument need to be explicitly optional (which is not the case). So, we have to pick a value.

Unfortunately, GTK_ICON_LOOKUP_GENERIC_FALLBACK is 2.12 and we need something that is available in 2.10. Can we use GTK_ICON_LOOKUP_NO_SVG?
Karl, would that be fine?

Comment 51

[Jean-Alexandre Anglès d'Auriac]

I think GTK_ICON_LOOKUP_USE_BUILTIN would be better. We know there are no builtin icons for Firefox, Thunderbird, Seamonkey or any other Xul application, so this flag doesn't do anything, I guess.

Comment 52

[Karl Tomlinson (:karlt)]

There's no need to use gtk_icon_theme_load_icon.

Only call gtk_window_set_icon_name(), and GTK will handle finding the available sizes, etc.

It looks like a gtk_window_set_icon_name(window, NULL) will also be needed in nsWindow::SetIcon().  Otherwise, if SetDefaultIcon is subsequently called again, gtk_window_set_icon_name() won't notice that a change is required.

(In reply to comment #43)
> The slightly tricky bit then is determining whether GTK will find icons for
> that name.  It looks like it should be reasonable to assume that, if
> gtk_icon_theme_get_icon_sizes() returns a non-empty array, then GTK will have
> some icons to use.

Easier/better than that is to check gtk_window_get_icon after calling gtk_window_set_icon_name() from SetDefaultIcon.

Comment 53

[Mounir Lamouri (:mounir)]

Jean-Alexandre, can you update your patch with recommendations from comment 52?

Comment 54

[Jean-Alexandre Anglès d'Auriac]

Err, I don't know how to handle theme change. If the user switches from a theme with a brandName icon to a theme without one, how can we go back to the default behavior ?
I think one workaround could be to add a brandName icon to the hicolor icon theme during install, but I don't know how to do that…

Comment 55

[Karl Tomlinson (:karlt)]

(In reply to comment #54)
> Err, I don't know how to handle theme change. If the user switches from a theme
> with a brandName icon to a theme without one, how can we go back to the default
> behavior ?

Oh, thanks, I hadn't thought about that.

> I think one workaround could be to add a brandName icon to the hicolor icon
> theme during install, but I don't know how to do that…

I don't know enough about that to comment.

One thing I thought about was using gtk_window_set_default_icon_list from nsNativeAppSupportUnix, which would provide a default for windows without
an icon_name and windows where there were no icons for the icon_name.

  In some ways that's tidy because nsWindow::SetDefaultIcon wouldn't need
  to worry about checking that there are icons for the icon_name.

  However, it is also unfortunate to load an icon that may not be used, which
  might have an impact on startup time.

  And it feels like it would be abusing the API a little.  Possibly we should
  be using gtk_window_set_default_icon_name and falling back to
  gtk_window_set_default_icon_list when there are no icons for the name.

Another possibility is to connect to the "changed" signal on the GtkIconTheme for each window that is using a themed icon (those for which SetDefaultIcon was called and found a brandName icon).

GtkWindow uses this to update the icon on theme changes.:

  g_signal_connect (icon_theme, "changed",
                    G_CALLBACK (update_themed_icon), window);

It might be easiest to simply call SetDefaultIcon from the our signal handler.
I expect that would handle switches both to and from themes with/without brandName icons.

Such a signal handler should be removed for windows with non-themed icons (when SetIcon is called).

Comment 56

[Jean-Alexandre Anglès d'Auriac]

Well, why not just adding the icons we want to use to hicolor, like I proposed in comment #54 ? It's the way most programs does it, and it seems much simpler, since theme change will be handled properly by gtk_window_set_icon_name, I guess.

Comment 57

[Karl Tomlinson (:karlt)]

I'm assuming you are asking me, but I can't tell you whether that is better or not because I don't know enough about that to comment.  I was mentioning another possible option, because it sounded like neither of us knew how to add an icon to hicolor.

What you say makes it sound good.

Would the icons be added to hicolor if not already present during startup?
That might be an option.

How do most programs do it?

(I don't know of any installation phase with Gecko apps.  It's more an untar and run situation, but I guess there must be some app-specific storage available if necessary as updates must use some kind of similar storage.)

Would GTK fallback to using the hicolor icon if the selected theme did not have an icon with matching name?

Comment 58

[Jean-Alexandre Anglès d'Auriac]

Actually, all icon theme inherits from hicolor, so yes, GTK always fallback to the hicolor icon if the current icon theme doesn't provide an icon.

So, there are actually two ways we can proceed. The first one is to put a firefox/whatever icon in /usr/share/icons/hicolor/sizexsize/apps, the second is to create a directory like /usr/share/firefox/icons/hicolor/sizexsize/apps, put a firefox/whatever icon in it, and add /usr/share/firefox/icons to the icon path.

Many applications (brasero, empathy, evolution, …) uses both solutions : they put their application icon in /usr/share/icons/hicolor, and all the icons they use in their interface in /usr/share/applicationname/icons/hicolor. This is because the launcher (i.e. the .desktop file which describe how the program should appear in the different menu) give the icon name, but can't modify the icon search path. 

Currently, the firefox icon used in it's launcher is stored in /usr/share/pixmaps.
I think the best way to do would be to move this icon to /usr/share/icons/hicolor/sizexsize/apps (and maybe provide different icon size). But if we do it this way, people who just untar and run won't get the right window icon. There is a way to bypass that by checking that the icon is present, and if it isn't, just adding it to a user-specific addition to the hicolor theme (we copy the icon in ~/.icon/hicolor/sizexsize/apps and we add a right index.theme in ~/.icon/hicolor/), but this seems dirty.

Comment 59

[Karl Tomlinson (:karlt)]

(In reply to comment #58)
> Actually, all icon theme inherits from hicolor, so yes, GTK always fallback to
> the hicolor icon if the current icon theme doesn't provide an icon.

Thanks.  Sounds good then.

> So, there are actually two ways we can proceed. The first one is to put a
> firefox/whatever icon in /usr/share/icons/hicolor/sizexsize/apps, the second is
> to create a directory like /usr/share/firefox/icons/hicolor/sizexsize/apps, put
> a firefox/whatever icon in it, and add /usr/share/firefox/icons to the icon
> path.
> 
> [...]
> 
> Currently, the firefox icon used in it's launcher is stored in
> /usr/share/pixmaps.
> I think the best way to do would be to move this icon to
> /usr/share/icons/hicolor/sizexsize/apps (and maybe provide different icon
> size).

I don't know whether or not mozilla-central has an installer script that can be modified to do this.  It seems that the file in /usr/share/pixmaps is copied there by scripts in the distribution packages.

> But if we do it this way, people who just untar and run won't get the
> right window icon. There is a way to bypass that by checking that the icon is
> present, and if it isn't, just adding it to a user-specific addition to the
> hicolor theme (we copy the icon in ~/.icon/hicolor/sizexsize/apps and we add a
> right index.theme in ~/.icon/hicolor/), but this seems dirty.

It looks like gtk_icon_theme_add_builtin_icon() can be used to add default icons during startup without needing to touch ~/.icon/hicolor.
It's unfortunate to load the files during startup when they may not be used, but it's got to be better than what we currently do, which is to load the files for every new window.

Perhaps, a better option would be to rearrange the file in chrome/icons/default/ (or add links) so that the directory can be added with gtk_icon_theme_append_search_path(), but that might be more work.

Comment 60

[Robert Kaiser]

We need to be able to run correctly (and with correct icons) when _not_ running any installation script. The way to "install" our official downloads is to to just unpack the .tar.bz2 and run the "firefox" (or "thunderbird", or "seamonkey) script in it. No other steps needed, and the application is supposed to act correctly in every way with just that.

Comment 61

[Jean-Alexandre Anglès d'Auriac]

I was trying to test a new patch using gtk_window_set_icon_name combined with gtk_icon_theme_append_search_path, but I can't get Firefox to compile anymore, since it is looking for Python 2.6, and I upgraded to Python 2.7…
Any suggestion ?

Comment 62

[Robert Kaiser]

(In reply to comment #61)
> it is looking for Python 2.6, and I upgraded to Python 2.7…
> Any suggestion ?

Yes, either clobber your objdir or at least delete config.* in there and re-run configure.

Comment 63

[Jean-Alexandre Anglès d'Auriac]

Thanks, worked perfectly !
Now, I have something that works perfectly, I just need to be able to find where the icons are, to use it as the argument for gtk_icon_theme_append_search_path. 

That is, 
- if it is an official pre-compiled version unpacked without any installation script, I must use /path/to/the/extracted/files/icons (and there must be a icons folder in the archive, with the right icons are the right place, but that should be too hard)
- if it is an installation from source with make install, I must use /usr/share/firefox/icons (or $prefix/share/firefox/icons, I guess), and we should also change make install so that it creates the right folder at the right place.

(I used the specification given here : http://live.gnome.org/ThemableAppSpecificIcons . BTW, in the long-term, it would provide a much better integration if all the icons from the GTK ui were put in an icons folder added to the hicolor icon theme. It would allow theme-makers to provide custom version of every icon of Firefox. For example, my icon theme provides a new window icon, and also a new tab icon, but they are not used by Firefox. With those changes, I could fix it in my theme.)

Comment 64

[Robert Kaiser]

Well, all the icons/ folder stuff basically breaks our cross-platform support probably significantly. Not sure if that's ideal.

Comment 65

[Jean-Alexandre Anglès d'Auriac]

Could you be more specific ? How does creating a specific directory for the gtk interface breaks cross-platform support ?

Comment 66

[Robert Kaiser]

Well, we're supposed to be able to have some kind of pluggable structure under chrome/icons that allows us to put in icons with the window names and have them used by the windows in some way across platforms. I never personally used this much, but I skimmed posts explaining this to people, not sure the details of the workings of it.

When it comes to icons being used in themes, that's even worse in that regard, but please let's put that discussion somewhere else than this bug.

Comment 67

[Jean-Alexandre Anglès d'Auriac]

I guess you are speaking of the SetIcon method. My patch only modifies the SetDefaultIcon method, that used to just call SetIcon with "default" as the argument. So I guess you can still put icons with the window name and have them used by the windows in some way across the platform.

Comment 68

[Karl Tomlinson (:karlt)]

(In reply to comment #66)
> Well, we're supposed to be able to have some kind of pluggable structure under
> chrome/icons that allows us to put in icons with the window names and have them
> used by the windows in some way across platforms.

Looking at

http://hg.mozilla.org/mozilla-central/file/39de140a8bdd/other-licenses/branding/firefox/Makefile.in#l74

it seems that the filenames are already quite different across platforms.

However, I had mis-assumed that there was only one directory structure that
would need rearranging (or supplementing with a new link structure).

As I think Robert was alluding to, XUL extensions can add their own themes
with their own directories of icons and these are would also need to be
considered.

Making gtk_icon_theme_append_search_path() work with icons from extensions
would pretty much require extensions to adapt to the new structure and also
require adding extension directories to the search path.

I suspect the easiest way to get something working here is going to be by
using the existing icon file finding code and
gtk_icon_theme_add_builtin_icon().  Given the existing ResolveIconName code in
nsBaseWidget, this is probably most easily done in widget/gtk2 code when the
icon is first required.

Comment 69

[Jean-Alexandre Anglès d'Auriac]

Do you mean that the problem comes from third-party theme that want to modify the window icon ? Or for interface icons like the big red one from AdBlock+ ?

Let's stick to window icon here, I'll open a bug for interface icon later.

Since I guess third-party theme uses SetIcon and not SetDefaultIcon, I think my method is still valid.

Comment 70

[Karl Tomlinson (:karlt)]

(In reply to comment #69)
> Do you mean that the problem comes from third-party theme that want to modify
> the window icon ?

I did mean the window icon.
I don't know for certain that there are XUL themes that change the default window icon, but the code (currently executed by SetDefaultIcon) looks like it provides for this:
http://hg.mozilla.org/mozilla-central/annotate/eaf6f9566f2e/widget/src/xpwidgets/nsBaseWidget.cpp#l1133
http://hg.mozilla.org/mozilla-central/annotate/eaf6f9566f2e/toolkit/xre/nsXREDirProvider.cpp#l680

Comment 71

[Jean-Alexandre Anglès d'Auriac]

Well, I'm sorry, I just don't understand anything about the code you are linking to. I never learned C++, I don't know how much about XUL, and if someone with better knowledge could take care of that bug, it would be great.

Comment 72

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

Comment on attachment 482018 [details] [diff] [review]
Improved patch

(bookkeeping)

Comment 73

[Jean-Alexandre Anglès d'Auriac]

Ok, I wish to make sure I understand the problem with the current patch.
The problem is that a theme might change the folders in which the XUL application looks for it default icon, and if we apply the current patch, then this modification of the search path won't have any effect. Is that it ?

Comment 74

[Karl Tomlinson (:karlt)]

(In reply to Jean-Alexandre Anglès d'Auriac from comment #73)

I guess you mean your patch referenced in comment 61 and comment 63.  I
haven't seen that patch so I'm guessing about how it might be working, but
yes, a Gecko theme/extension might add folders in which the app looks for its
default and non-default icons.  I have trouble imagining how that would
continue to work if nsBaseWidget::ResolveIconName() is no longer used.

However, I see nsCocoaWindow (for Mac OS) doesn't implement nsIWidget::SetIcon() at all, and Mac windows don't have icons.  Gecko extensions therefore can't modify window icons in a cross-platform manner.  On Mac, Thunderbird modifies its launcher icon with the message count, but there must be some other code that does this.

There may be very little value in continuing to support changing the default icon through Gecko extensions.
Maybe we should try switching from nsBaseWidget::ResolveIconName() to
gtk_icon_theme_append_search_path(), if that is what you have in mind.

Comment 75

[Jean-Alexandre Anglès d'Auriac]

Yes, I was.
If a patch that works even though not supporting changing the default icon the old way can be accepted, that's good news ! I'll resume working on working on a patch using gtk_icon_theme_append_search_path() and gtk_window_set_icon_name() then.

Comment 76

[Jean-Alexandre Anglès d'Auriac]

I don't know if I should put the icons in NS_APP_CHROME_DIR, or in one of the directories in NS_APP_CHROME_DIR_LIST, or elsewhere.
Currently, it seems to me that the code search for icons in both NS_APP_CHROME_DIR and NS_APP_CHROME_DIR_LIST.
Cf http://hg.mozilla.org/mozilla-central/annotate/eaf6f9566f2e/widget/src/xpwidgets/nsBaseWidget.cpp#l1112

Which one makes more sense ?

Comment 77

[Karl Tomlinson (:karlt)]

The default icons are currently in $(DIST)/bin/chrome/icons/default.
Without any other influencing factors, I'd aim for something similar.

http://hg.mozilla.org/mozilla-central/annotate/086dea3f0bad/browser/app/Makefile.in#l194

Comment 78

[Jean-Alexandre Anglès d'Auriac]

Actually, we need to put them in $(DIST)/bin/chrome/hicolor/sizexsize/apps.
And there is also another issue I didn't think about.
The icon names should be changed to match GetBrandName put in lowercase. Is it a problem ?

Comment 79

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that uses gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name

Comment 80

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

This patch works without any change to the Makefile or in the place or name of the icons.
I did make it also provide GTK theming for non-default icon, however we can remove that, or add a moz- prefix to the icon name, if it causes problem.

Comment 81

[Karl Tomlinson (:karlt)]

Comment on attachment 575978 [details] [diff] [review]
Patch that uses gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name

This code is setting up GTK to look for default icons under NS_APP_CHROME_DIR.
Such an approach should be fine if the appropriate Makefile changes can be
made to alter the icon install locations so that GTK will find the distributed
icons.

However, the purpose of nsWindow::SetDefaultIcon() is to make that particular
window have the default icon.  With such an approach,
gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name should
be called only once instead of for each window.

There should be some code to ensure that SetDefaultIcon() undoes the
effects of a previous SetIcon() call.
I assume gtk_window_set_icon_list(GTK_WINDOW(mShell), NULL) would do that.
But I suspect SetIcon() should also be modified to use
gtk_window_set_icon_name() so that all icons will be installed in the same kind of directory structure.

Note also that nsXULWindow expects SetIcon("default") to select the default
icon.
http://hg.mozilla.org/mozilla-central/annotate/cb7297c7297c/xpfe/appshell/src/nsXULWindow.cpp#l1403

>+    nsXPIDLString brandName;
>+    GetBrandName(brandName);
>+    NS_ConvertUTF16toUTF8 cBrand(brandName);
>+    gchar* iconName = g_utf8_strdown(cBrand.get(), -1);

iconName would need to be freed, but instead use ToLowerCase() from
nsREadableUtils to change case in place.

>+    gtk_window_set_default_icon_name("iconName");

ITYM the variable iconName instead of the literal string.

Comment 82

[Karl Tomlinson (:karlt)]

Comment on attachment 576886 [details] [diff] [review]
Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

This gtk_icon_theme_add_builtin_icon approach is also fine if that is easier.

It should be possible to share the code rather than copying SetIcon code to
SetDefaultIcon.

Many of my comments on the gtk_icon_theme_append_search_path patch also apply
here.

Comment 83

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

Comment 84

[Jean-Alexandre Anglès d'Auriac]

(In reply to Karl Tomlinson (:karlt) from comment #81)
> However, the purpose of nsWindow::SetDefaultIcon() is to make that particular
> window have the default icon.  With such an approach,
> gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name should
> be called only once instead of for each window.
When should it be done ? I couldn't find where gtk_init() is called. I tried to do it in moz_gtk_init, but it is in gtk2drawing.c, and I couldn't use the C++ fonction needed to get the NS_APP_CHROME_DIR path.

Comment 85

[Karl Tomlinson (:karlt)]

(In reply to Jean-Alexandre Anglès d'Auriac from comment #84)
> (In reply to Karl Tomlinson (:karlt) from comment #81)
> > With such an approach,
> > gtk_icon_theme_append_search_path and gtk_window_set_default_icon_name should
> > be called only once instead of for each window.
> When should it be done ?

The first time SetIcon or SetDefaultIcon is called should be fine.  You can
use a static variable to record whether or not this has already been done.

Comment 86

[Karl Tomlinson (:karlt)]

Comment on attachment 579689 [details] [diff] [review]
Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

Similarly with this approach, gtk_icon_theme_add_builtin_icon should be called
only once for each iconName.

I don't see a reasonable way to find out from GTK what we have added, so we'll
have to keep track of that ourselves.

I suspect it would be easiest to do this only for the default icon, using a
single static variable.  It would be possible to do this for all icons by
keeping a set of icons that have been added.

>+    if (!g_strcmp0("default",iconName.get())){

iconName.EqualsLiteral("default")
(g_strcmp0 is not available in the GLib against which we build.)

>+    gboolean foundicon = FALSE;

Use the C++ bool/true/false where practical, saving gboolean just for
interaction with GLib.  bool is a real boolean and its use follows Gecko
convention.  gboolean is actually an integer.

>+            GdkPixbuf *icon = gdk_pixbuf_new_from_file(path.get(), NULL);

Check for null icon before adding it.
Need to unref after adding, I expect.

Comment 87

[Jean-Alexandre Anglès d'Auriac]

(In reply to Karl Tomlinson (:karlt) from comment #85)
> The first time SetIcon or SetDefaultIcon is called should be fine.  You can
> use a static variable to record whether or not this has already been done.
Well, if I want something that could be reusable for bug #705070, it would need to be done beforehand, during the GTK initialization,  I guess


I think that maybe, instead of using a static boolean, we could use gtk_icon_theme_has_icon to check if adding a builtin is required, what do you think about that ? Therefore, we could do it for every icon, and we only do it if really needed, which should be very rare (almost every distribution adds a firefox and thunderbird icon to the icon theme, so that it can be used for the .desktop launcher).
Updated patch coming tomorow when I've got the time to test-compile it.

Comment 88

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

Comment 89

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

I updated the wrong patch.

Comment 90

[Karl Tomlinson (:karlt)]

Comment on attachment 579933 [details] [diff] [review]
Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

I like this approach, thank you.
It doesn't address the situation with existing windows when the user switches from a theme with a brandName icon to a theme without one (comment 54), but this approach is better in other ways, and more code can be added in the future if that situation turns out to be important.

>+static void GetBrandName(nsXPIDLString& brandName);

Can you move this to the top of the file with other forward declarations,
please.

>+    NS_ConvertUTF16toUTF8 iconName(aIconSpec);
>+    
>+    if (iconName.EqualsLiteral("default")){
>+        nsXPIDLString brandName;
>+        GetBrandName(brandName);
>+        NS_ConvertUTF16toUTF8 brandName8(brandName);
>+        ToLowerCase(brandName8);
>+        iconName = brandName8;
>+        }

I think we can save a copy and do this slightly more efficiently:

nsCAutoString iconName
if (iconName.EqualsLiteral("default")){
    nsXPIDLString brandName;
    GetBrandName(brandName);
    AppendUTF16toUTF8(brandName, iconName);
    ToLowerCase(iconName);
} else {
    AppendUTF16toUTF8(aIconSpec, iconName);
}

>+    bool foundicon = false;

Normally Gecko uses camelCase for variables, so this would be "foundIcon".

>+    if(!gtk_icon_theme_has_icon(gtk_icon_theme_get_default(),iconName.get())){

Need a space after "if", after "," and before "{".
But how about doing this to make the "else" unnecessary:

    bool foundIcon =
        gtk_icon_theme_has_icon(gtk_icon_theme_get_default(), iconName.get());
    if (!foundIcon) {
        ...
    }

>-            iconList.AppendElement(path);

>-    return SetWindowIconList(iconList);

iconList and SetWindowIconList are now unused and so can be removed.

>+                if(icon){
>+                    gtk_icon_theme_add_builtin_icon(iconName.get(),gdk_pixbuf_get_height(icon),icon);

Spaces needed to follow file style here too, and split the argument list across lines to keep within 80 columns.

>+                    g_free(icon);

GdkPixbuf is a GObject, so use g_object_unref instead of g_free.

Comment 91

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

I think I should move g_signal_connect(gtk_icon_theme_get_default(), "changed", G_CALLBACK(checkIconOnIconThemeChange), mShell) somewhere else, in some part of the code that is executed only once, but I have no idea where.

Comment 92

[Karl Tomlinson (:karlt)]

Comment on attachment 580190 [details] [diff] [review]
Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

(In reply to Jean-Alexandre Anglès d'Auriac from comment #91)
> I think I should move g_signal_connect(gtk_icon_theme_get_default(),
> "changed", G_CALLBACK(checkIconOnIconThemeChange), mShell) somewhere else,
> in some part of the code that is executed only once, but I have no idea
> where.

This may be tricky to get right and I don't think it is important, so I
suggest leaving this out of this patch.  We can consider a subsequent patch if
someone has tested that it works.

>
>+static void GetBrandName(nsXPIDLString& brandName);
>+void checkIconOnIconThemeChange(GtkIconTheme *icontheme, GtkWidget *window);
> /* callbacks from widgets */

Leave the blank line before the "callbacks from widgets" section, please.

>+    if (aIconSpec.EqualsLiteral("default")){

I see I left out the space between ")" and "{" in my own copy-paste.
Can you add that in, please?

>+    if (!foundIcon) {
>     // Look for icons with the following suffixes appended to the base name.

Can you indent the comment lines too, please?

The now-unused SetWindowIconList() method should be removed.

Comment 93

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

Comment 94

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that add the builtin icon if needed on theme change

I guess the right place to add this signal is in the fonction that create the window, just after the default icon has been set up.
I actually tested the patch. I had to manually remove all the firefox icons from the hicolor theme, and remove /usr/share/icons/hicolor/icon-theme.cache (finding that took me quite some time), to test it. At the beginning, it was causing firefox to segfault on exit, that's why I replaced “if(!(iconTheme && window))” by “if(!(GTK_IS_ICON_THEME(iconTheme) && GTK_IS_WINDOW(window)))”.
I switched themes a few dozen times, and it seemed to work pretty much fine.
If you have any suggestion on how to test it further, tell me.

Comment 95

[Karl Tomlinson (:karlt)]

Comment on attachment 580359 [details] [diff] [review]
Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

Looks good.  Thank you for tidying this up.

Comment 96

[Jean-Alexandre Anglès d'Auriac]

Ok, so referring to this article :
https://developer.mozilla.org/en/Hacking_Mozilla
I should now either ask you to push the patch, or add checkin-needed to the keyword of the bug. Is that right ? Which one should I do ? Should I wait to see if the other patch is also alright, to combine them into one patch before pushing them ?

Comment 97

[Karl Tomlinson (:karlt)]

Comment on attachment 580431 [details] [diff] [review]
Patch that add the builtin icon if needed on theme change

(In reply to Jean-Alexandre Anglès d'Auriac from comment #94)
> I guess the right place to add this signal is in the fonction that create
> the window, just after the default icon has been set up.

The Create method is a suitable place, yes.  SetIcon() was probably a suitable
place too, but here it is clear that the call happens before gtk_widget_realize().  (I don't think it matters whether it is before or after the SetDefaultIcon(), but I assume it is important that it is before gtk_widget_realize - see below.)

>+checkIconOnIconThemeChange(GtkIconTheme *iconTheme, GtkWidget *window)

Gecko uses CapitalizedCamelCase for function names and uncapitalizedCamelCase
for variable names, so please capitalize the first "C" of check. 

Please use "static" to give this function internal linkage.

>+    const gchar* uiconName = gtk_window_get_icon_name(GTK_WINDOW(window));
>+    if (!gtk_icon_theme_has_icon(iconTheme,uiconName)){
>+        nsWindow *nsWindow = get_window_for_gtk_widget(window);
>+        nsWindow->SetIcon(NS_LITERAL_STRING(iconName));
>+     }

Apologies for jumping to the conclusion that this wasn't tested.  I guess the
"u" before "iconName" was to make NS_LITERAL_STRING(iconName) work for you,
though I'm still not clear what this actually does because this is not the
intended purpose of NS_LITERAL_STRING.

NS_LITERAL_STRING is actually intended for strings that are known at compile
time.  Here you want to convert the GTK icon_name from utf-8 to utf-16.

However, it is more complicated than that because the default icons are stored
under the name "default", while the GTK icon_name is the brand name.

> At the beginning, it was
> causing firefox to segfault on exit, that's why I replaced “if(!(iconTheme
> && window))” by “if(!(GTK_IS_ICON_THEME(iconTheme) &&
> GTK_IS_WINDOW(window)))”.

It's actually mostly getting lucky that this resolved the crash.

When the nsWindow is destroyed, window is deleted and so the memory at which
|window| points is being recycled for other use.  It may be a new window; it
could even get marked as inaccessible.

The signal handler needs to be removed before the window is deleted.  See
calls to g_signal_handlers_disconnect_by_func in nsWindow.cpp for examples of
how that is done.

Interestingly, it looks like the subsequent the gtk_window_set_icon_name() with the same name behaves like a no-op.  It does not cause GTK to search again for icons.  GTK also connects to the "changed" signal and searches again for icons when it receives this signal.  Because the signal handler in this patch is registered before the signal handler that GTK registers, it runs first and then GTK's signal handler picks up the changes.

> I switched themes a few dozen times, and it seemed to work pretty much fine.
> If you have any suggestion on how to test it further, tell me.

Did you start with a theme that has a "firefox" icon installed, and then
switch to a theme that does not have a "firefox" icon?

Comment 98

[Jean-Alexandre Anglès d'Auriac]

(In reply to Karl Tomlinson (:karlt) from comment #97)
> Comment on attachment 580431 [details] [diff] [review]
> Patch that add the builtin icon if needed on theme change
> 
> The Create method is a suitable place, yes.  SetIcon() was probably a
> suitable
> place too, but here it is clear that the call happens before
> gtk_widget_realize().  (I don't think it matters whether it is before or
> after the SetDefaultIcon(), but I assume it is important that it is before
> gtk_widget_realize - see below.)
SetIcon() seemed bad because we don't want to call g_signal_connect 4 or 5 times for each new window, and then call it once more every time the user changed his icon theme.
> However, it is more complicated than that because the default icons are
> stored under the name "default", while the GTK icon_name is the brand name.
You are right. If the window icon is brandname, there is actually no way to find out if it was set initially set as "default" or as "brandname".
This is problematic.
I think a different approach can be better. What do you think about creating a completely empty GTK theme using gtk_icon_theme_set_search_path, to check if we already added builtin icons ?
I tried 
  emptypath[0]=NULL;
  GtkIconTheme *virgin = gtk_icon_theme_new();
  gtk_icon_theme_set_search_path(virgin,emptypath,1);
and it effectively gives an icon theme containing only the builtin icons.
> Did you start with a theme that has a "firefox" icon installed, and then
> switch to a theme that does not have a "firefox" icon?
Yes, and it seemed to work.
But has you have pointed, it shouldn't.
I'm getting confused here. I must have made a mistake somewhere.

Comment 99

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that add the builtin icon once and only once

Tested, with extra care this time. Seemed to work fine.

Comment 100

[Karl Tomlinson (:karlt)]

(In reply to Jean-Alexandre Anglès d'Auriac from comment #98)
> I think a different approach can be better. What do you think about creating
> a completely empty GTK theme using gtk_icon_theme_set_search_path, to check
> if we already added builtin icons ?
> I tried 
>   emptypath[0]=NULL;
>   GtkIconTheme *virgin = gtk_icon_theme_new();
>   gtk_icon_theme_set_search_path(virgin,emptypath,1);
> and it effectively gives an icon theme containing only the builtin icons.

This approach should work.  I think the n_elements parameter for
gtk_icon_theme_set_search_path should be 0, which should mean that the theme
itself adds no filesystem reading/scanning.

However, this approach misses out on the advantage that the previous patch had
that it skipped loading the files if they were not used.

Assuming that most distribution installs add icons to the hicolor theme, I
expect most people will not need these files.

There are some people who will only install from another source
(e.g. mozilla.com), but even for them, the issue that this is addressing is
not going to happen so frequently or be so major that it is worth the cost to
everyone.

Comment 101

[Karl Tomlinson (:karlt)]

Comment on attachment 580359 [details] [diff] [review]
Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

This is clear win IMO, so let's try to get this in ASAP.

Comment 102

[Jean-Alexandre Anglès d'Auriac]

(In reply to Karl Tomlinson (:karlt) from comment #100)
> However, this approach misses out on the advantage that the previous patch
> had
> that it skipped loading the files if they were not used.
Well, yes, but they are only added once. I think the time needed to look for the file on the filesystem is not important (the previous code was looking for those files 3 or 4 times for each window, it seems to me). If it was important, then maybe this will even improve the performance.
But you are right that it is not addressing a common issue, especially XUL users switching theme already have bigger issues since the icons in gnomestripe don't change. So, the question is : do we want to make icon theme change “on the fly” possible, or is it fine if it requires to restart XUL ?

Comment 103

[Jean-Alexandre Anglès d'Auriac]

Attachment: Patch that add the builtin icon once and only once

Just fixed gtk_icon_theme_set_search_path(virgin,emptypath,1); to gtk_icon_theme_set_search_path(virgin,emptypath,0);

Comment 104

[Mounir Lamouri (:mounir)]

Comment on attachment 580359 [details] [diff] [review]
Patch that uses gtk_icon_theme_add_builtin_icon and gtk_window_set_default_icon_name

Next time, it would be great to have a commit message :)

Comment 105

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/ffea93b21d4d

Comment 106

[Karl Tomlinson (:karlt)]

(In reply to Jean-Alexandre Anglès d'Auriac from comment #102)
> do we want to make icon
> theme change “on the fly” possible, or is it fine if it requires to restart
> XUL ?

Sorry, I missed this.  For some reason, I don't seem to be getting email notifications for your (and only your) comments.

On the fly changes are nice, and AIUI will actually work for most users/situations with the patch that has landed.  The patch that has landed has fixed this bug as originally reported and is scheduled to ship in Firefox 11, so I'll close this bug.

If you want to address the issue for people without an icon in the hicolor theme, that can be addressed in new bug report.  Perhaps an approach similar to attachment 580431 [details] [diff] [review] but with the default icons renamed to match the brand names ("firefox", "nightly", and whatever is appropriate for beta and aurora) would work without requiring loading at startup any files that are not needed.  However, I suspect there are more important things that could be fixed.

Comment 107

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

I think this change broke the icon entirely when --enable-official-branding is used.  With an --enable-official-branding build, I'm seeing the default process icon rather than a Firefox icon.

It's probably worth checking that it works with each of:

ac_add_options --enable-official-branding
ac_add_options --with-branding=browser/branding/aurora
ac_add_options --with-branding=browser/branding/nightly
ac_add_options --with-branding=browser/branding/official
ac_add_options --with-branding=browser/branding/unofficial

Comment 108

[Jean-Alexandre Anglès d'Auriac]

My testing build was with --enable-official-branding, and it worked.
Does your icon theme include a firefox icon ? Does the problem happens in both situations (with and without a firefox icon in the theme) ? Does it also happens when applying the patch that add the builtin icon once and only once ?

Comment 109

[Karl Tomlinson (:karlt)]

I'm seeing the nightly and aurora icons in official builds.
Branding configure arguments are not listed in about:buildconfig, but I assume they are used in these builds.

Comment 110

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

(In reply to Jean-Alexandre Anglès d'Auriac from comment #108)
> Does your icon theme include a firefox icon ?

How would I tell?  I'm running GNOME 2 on Ubuntu 11.04.

And shouldn't things still work when it doesn't?

Comment 111

[Jean-Alexandre Anglès d'Auriac]

Yes, it definitely should, as you could have tell if you had looked at the previous comments or at the code. I'm trying to track down where the bug is, that's why I asked you this question.
Can you please compile that, launch it, and tell me the result (i.e. does it say that there is a firefox icon in your theme, and does the window use it ?)

#include <gtk/gtk.h>

void
destroy (void)
{
  gtk_main_quit ();
}

int
main (int argc, char *argv[])
{
  GtkWidget *window;
  gtk_init (&argc, &argv);

  if (gtk_icon_theme_has_icon(gtk_icon_theme_get_default(), "firefox"))
    printf("Default theme has an firefox icon\n");
  else
    printf("Default theme has no firefox icon\n");
  window = gtk_window_new (GTK_WINDOW_TOPLEVEL);
  gtk_window_set_icon_name (GTK_WINDOW(window), "firefox");
  gtk_widget_show (window);
  g_signal_connect (window, "destroy",G_CALLBACK (destroy), NULL);

  gtk_main ();

  return 0;
}

Comment 112

[Chris Coulson]

This seems to cause bug 714068

Comment 113

[Jean-Alexandre Anglès d'Auriac]

(In reply to Chris Coulson from comment #112)
> This seems to cause bug 714068

Thanks, I'll look for what I can do about bug 714068