Last Comment Bug 495250 - hook up 22/24/256px icons on Linux for gnome and KDE (was: Refresh Linux Firefox icons)
: hook up 22/24/256px icons on Linux for gnome and KDE (was: Refresh Linux Fire...
Status: NEW
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: 3.5 Branch
: All Linux
: -- enhancement (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
:
Mentors:
Depends on:
Blocks: 492431
  Show dependency treegraph
 
Reported: 2009-05-28 09:02 PDT by David Dahl :ddahl
Modified: 2014-06-20 09:09 PDT (History)
22 users (show)
mbeltzner: blocking‑firefox3.5-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
22x22 no shadow (1.59 KB, image/png)
2009-06-06 02:59 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
22x22 with shadow (1.61 KB, image/png)
2009-06-06 03:04 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
24x24 no shadow (1.81 KB, image/png)
2009-06-06 03:05 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
24x24 with shadow (1.84 KB, image/png)
2009-06-06 03:07 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
App icon for linux, all resolutions (115.01 KB, application/x-zip-compressed)
2009-06-06 12:02 PDT, Alex Faaborg [:faaborg] (Firefox UX)
no flags Details
WIP - trouble with nsWindow::SetIcon (1.02 MB, patch)
2009-06-07 12:50 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
Proper SetIcon() (1.02 KB, patch)
2009-06-07 17:15 PDT, Michael Ventnor
no flags Details | Diff | Splinter Review
v 0.1 Linux Icons for linux (1.02 MB, patch)
2009-06-07 17:44 PDT, David Dahl :ddahl
dao+bmo: review-
Details | Diff | Splinter Review
v0.2 Linux Icons - removed additional icons not needed in this patch (310.78 KB, patch)
2009-06-07 19:57 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v0.3 added 22x22 and 24x24 to nsWindow.cpp (310.79 KB, patch)
2009-06-07 20:04 PDT, David Dahl :ddahl
dao+bmo: review-
Details | Diff | Splinter Review
v 0.4 removed changes to browser/app (169.90 KB, patch)
2009-06-08 12:48 PDT, David Dahl :ddahl
dao+bmo: review-
Details | Diff | Splinter Review
nsWindow.cpp changes for linux only (1.38 KB, patch)
2009-06-08 15:35 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
WIP patch - started over (106.46 KB, patch)
2009-06-08 19:26 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.1.1 Linux Icons - Trunk Patch (364.62 KB, patch)
2009-06-10 09:24 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.1.2 Trunk Patch (114.54 KB, patch)
2009-06-10 11:01 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.1 Branch / 191 patch (114.03 KB, patch)
2009-06-11 11:33 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.2 Trunk Patch (114.70 KB, patch)
2009-06-12 08:58 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review
v 0.2 Branch / 191 Patch (114.02 KB, patch)
2009-06-12 08:59 PDT, David Dahl :ddahl
no flags Details | Diff | Splinter Review

Description David Dahl :ddahl 2009-05-28 09:02:02 PDT
Update linux theme with new icons
Comment 1 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-05 22:52:37 PDT
Note related bug 484064 for updating the Thunderbird app icon for Linux.
Comment 2 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 00:09:10 PDT
In email Kalle Pearson writes:

>If you ask me, the best way would be to ship 16,22,32,48,128 and 256
>sizes of the icon "firefox" in the .png format and install them
>in /usr/share/icons/hicolor/$size/apps/

>That would make it scale nicely.
Comment 3 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 00:12:07 PDT
Monreal adds:

>We also need 24x24

and mventnor comments:

>Don't use XPM (although everyone said this already :) ), it's heavily deprecated >in favour of PNG.
>I'd imagine it's just a case of adding those new resolutions that Kalle >suggested then changing a couple of Makefiles. The XPMs really should be deleted >if at all possible.
Comment 4 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 01:07:04 PDT
We might remove the xpm files in bug 495250 along with some other legacy files that are no longer needed.
Comment 5 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 01:07:59 PDT
Just to clarify: does removing the xpm files change our backwards compatibility with older desktop environments at all?  Or is packaging both xpm and png files simply redundant?
Comment 6 Michael Ventnor 2009-06-06 01:57:07 PDT
We don't work with GTK+ < 2.10, so we shouldn't worry about XPM if backwards compatibility is the only concern.
Comment 7 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 02:14:32 PDT
Anyone know what the deal is with this file:

http://mxr.mozilla.org/mozilla-central/source/other-licenses/branding/firefox/document.png

It's using the pre-firefox 1 branding, and an XP style page, but since it's not .icns or .ico, it seems like it might have at some point been (or is currently) related to linux.
Comment 8 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 02:29:31 PDT
So I screwed up and didn't realize that we needed 22x22 and 24x24, both of these are going to be a Bicubic Sharper reduction of the pixel polished 32x32.  If we do an RC2 we can potentially get new files at these resolutions with manual pixel hinting.
Comment 9 Michael Ventnor 2009-06-06 02:37:56 PDT
(In reply to comment #7)
> Anyone know what the deal is with this file:
> 
> http://mxr.mozilla.org/mozilla-central/source/other-licenses/branding/firefox/document.png

I think it's unused. MIME icons in Linux are provided by the toolkit and have no app branding.

(In reply to comment #8)
> So I screwed up and didn't realize that we needed 22x22 and 24x24, both of
> these are going to be a Bicubic Sharper reduction of the pixel polished 32x32. 
> If we do an RC2 we can potentially get new files at these resolutions with
> manual pixel hinting.

That should be fine, although I don't think an icon at that size would have a shadow underneath it if the shadow is in the 32x32 version, as that is what seems to happen with most icons on my desktop.
Comment 10 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 02:43:04 PDT
>That should be fine, although I don't think an icon at that size would have a
>shadow underneath it if the shadow is in the 32x32 version, as that is what
>seems to happen with most icons on my desktop.

We also have all of the standard resolutions without a shadow.  What resolutions on linux should be shadow free? (22, 24 and 16?)
Comment 11 Michael Ventnor 2009-06-06 02:47:21 PDT
(In reply to comment #10)
> We also have all of the standard resolutions without a shadow.  What
> resolutions on linux should be shadow free? (22, 24 and 16?)

Yeah, that seems right after looking at /usr/share/icons.
Comment 12 Michael Monreal [:monreal] 2009-06-06 02:52:05 PDT
The (now outdated) tango styleguide does not really talk much about shadows but normally we have slight shadows on the 22/24px variant, just not on the 16px one.

Note that 22/24px is commonly created from the same source artwork, the difference being a 1-pixel border added to the 24px variant (for historical reason e.g. KDE3 which used 22px for toolbars).
Comment 13 Michael Ventnor 2009-06-06 02:54:34 PDT
(In reply to comment #12)
> The (now outdated) tango styleguide does not really talk much about shadows but
> normally we have slight shadows on the 22/24px variant, just not on the 16px
> one.

Does this apply for app icons? In my entire Applications menu I only notice about 3 or 4 icons that have any sort of noticeable shadow.
Comment 14 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 02:59:49 PDT
Created attachment 381918 [details]
22x22 no shadow

I'll attach 22 and 24 so that you can see what they look like with and without a shadow.
Comment 15 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 03:04:10 PDT
Created attachment 381919 [details]
22x22 with shadow
Comment 16 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 03:05:54 PDT
Created attachment 381920 [details]
24x24 no shadow
Comment 17 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 03:07:01 PDT
Created attachment 381921 [details]
24x24 with shadow
Comment 18 Michael Monreal [:monreal] 2009-06-06 03:07:24 PDT
(In reply to comment #13)
> Does this apply for app icons? In my entire Applications menu I only notice
> about 3 or 4 icons that have any sort of noticeable shadow.

I don't think we have a strict policy. 16px has no shadow, 32px and up have a
shadow. Jakub, Andreas: what about 22/24?

The icons most similar to the firefox icon (internet category and generic web
browser icon) both have a slight shadow in GNOME's default theme btw but I
don't have any strong feelings either way.
Comment 19 Michael Ventnor 2009-06-06 03:10:44 PDT
(In reply to comment #18)
> The icons most similar to the firefox icon (internet category and generic web
> browser icon) both have a slight shadow in GNOME's default theme btw but I
> don't have any strong feelings either way.

Ah yes, I do see that.

Looking at the uploaded icons, they both look nice, now I can't decide. :) Although I think the shadow is a bit too dark compared with the shadow of Epiphany's and the other icons?
The existing Firefox icon at this size doesn't have a shadow.
Comment 20 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-06 12:02:01 PDT
Created attachment 381970 [details]
App icon for linux, all resolutions

This zip file contains the app icon at 16, 22, 24, 32, 48, 128 and 256
Comment 21 Michael Monreal [:monreal] 2009-06-06 14:15:18 PDT
Great... I was skeptical about the icon refresh but those icons really look great in action. Even together with Tango :)

I was going to vote for the 22px-with-shadow variant, but after seeing it in the menus there's something odd about it... the shadow seems to add to the tail, making it look thicker at the bottom and taking away some roundness. Maybe making the shadow slightly larger (to the sides) would help but right now I also like the no-shadow version.
Comment 22 Michael Ventnor 2009-06-06 17:29:05 PDT
Ah, also you need to add the suffixes to nsWindow::SetIcon (and remove the mentions of XPM hopefully)

extensions[6][7] should become

extensions[7][9] = {".png", "16.png", "22.png", "24.png", "32.png", "48.png", "128.png", "256.png"}

and the code underneath it that break;s should be deleted (since even then it mentions XPM is deprecated!!!)
Comment 23 Michael Ventnor 2009-06-06 17:30:43 PDT
Er, maybe I have the numbers in extensions[][] mixed up...
Never mind, I'm sure ddahl knows what to do :)
Comment 24 David Dahl :ddahl 2009-06-07 12:47:10 PDT
Actually, I am not sure what to do, I cannot get it to build if I add the new icon suffixes:

NS_IMETHODIMP
nsWindow::SetIcon(const nsAString& aIconSpec)
{
    if (!mShell)
        return NS_OK;

    nsCOMPtr<nsILocalFile> iconFile;
    nsCAutoString path;
    nsTArray<nsCString> iconList;

    // Look for icons with the following suffixes appended to the base name.
    // The last two entries (for the old XPM format) will be ignored unless
    // no icons are found using the other suffixes. XPM icons are depricated.

    const char extensions[6][9] = { ".png", "16.png", "32.png", "48.png", 
                                    "128.png", "256.png", ".xpm", "16.xpm" };

    for (PRUint32 i = 0; i < NS_ARRAY_LENGTH(extensions); i++) {
        // Don't bother looking for XPM versions if we found a PNG.
        if (i == NS_ARRAY_LENGTH(extensions) - 2 && iconList.Length())
            break;

        nsAutoString extension;
        extension.AppendASCII(extensions[i]);

        ResolveIconName(aIconSpec, extension, getter_AddRefs(iconFile));
        if (iconFile) {
            iconFile->GetNativePath(path);
            iconList.AppendElement(path);
        }
    }

    // leave the default icon intact if no matching icons were found
    if (iconList.Length() == 0)
        return NS_OK;

    return SetWindowIconList(iconList);
}

i've tried "extensions[6][7]" after removing the 2 .xpms and adding the 2 new pngs.

Also leaving in the xpms and adding 2 pngs, like this: "extensions[6][9]"

It seems like the makefile cannot find the icons.
Comment 25 David Dahl :ddahl 2009-06-07 12:50:44 PDT
Created attachment 382053 [details] [diff] [review]
WIP - trouble with nsWindow::SetIcon

Just adding my work in progress patch, in case someone can tell what is wrong with nsWindow::SetIcon.
Comment 26 Michael Ventnor 2009-06-07 17:15:40 PDT
Created attachment 382069 [details] [diff] [review]
Proper SetIcon()

This works; it's just that C does two-dimensional arrays in a confusing way.
And, seriously, kill XPM. :)
Comment 27 David Dahl :ddahl 2009-06-07 17:44:05 PDT
Created attachment 382070 [details] [diff] [review]
v 0.1 Linux Icons for linux

I am going to run a tryserver build momentarily for viewing in action
Comment 28 Michael Ventnor 2009-06-07 17:50:47 PDT
Is there a reason you're not including the 22x22 and 24x24? If this is what the final SetIcon will look like then you should change it to extensions[6][8].
Comment 29 Michael Ventnor 2009-06-07 17:53:07 PDT
And the rule seems to be extensions[x][y] where x is the number of strings in the array and y is the length of the longest string in the array + 1 (for the NUL).
Comment 30 Dão Gottwald [:dao] 2009-06-07 18:00:54 PDT
Comment on attachment 382070 [details] [diff] [review]
v 0.1 Linux Icons for linux

browser/app/ and browser/branding/unofficial/ aren't expected to contain the Firefox icon.

Why is this patch touching wizHeader.bmp & Co.?
Comment 31 David Dahl :ddahl 2009-06-07 19:38:17 PDT
I really did not know which icons to include - so I added all of them. Let me know which to include, and I will remove the additional icons.
Comment 32 David Dahl :ddahl 2009-06-07 19:57:03 PDT
Created attachment 382076 [details] [diff] [review]
v0.2 Linux Icons - removed additional icons not needed in this patch

Ok, got rid of the extras images. What was i thinking?:)
Comment 33 David Dahl :ddahl 2009-06-07 20:04:53 PDT
Created attachment 382077 [details] [diff] [review]
v0.3 added 22x22 and 24x24 to nsWindow.cpp

this patch is also addressing comment 28
Comment 34 Dão Gottwald [:dao] 2009-06-08 01:04:36 PDT
Comment on attachment 382077 [details] [diff] [review]
v0.3 added 22x22 and 24x24 to nsWindow.cpp

Again, I don't see why you're patching files in browser/app/ and browser/branding/unofficial/. See http://mxr.mozilla.org/mozilla-central/source/browser/app/default32.png and http://mxr.mozilla.org/mozilla-central/source/browser/branding/unofficial/default32.png for instance -- no Firefox icon there.

Shouldn't the 22 and 24 icons also be added in browser/app/Makefile.in?
Comment 35 Michael Ventnor 2009-06-08 02:51:06 PDT
As well as the 128 one. And the XPMs should be removed since they won't be used anymore.

The Firefox icons only seem to be under other-licenses/branding/firefox/.
Comment 36 Jakub Steiner 2009-06-08 08:11:51 PDT
> I don't think we have a strict policy. 16px has no shadow, 32px and up have a
> shadow. Jakub, Andreas: what about 22/24?

Unless it poses a problem (such as in action icons 'add' and 'remove') we do have a shadow for the 22x22/24x24px size.
Comment 37 David Dahl :ddahl 2009-06-08 12:48:38 PDT
Created attachment 382169 [details] [diff] [review]
v 0.4 removed changes to browser/app

reverted icon changes in browser/app
Comment 38 David Dahl :ddahl 2009-06-08 13:20:25 PDT
very strange - I think we need to add a Minefield icon to browser/app/default256.png for this to build properly.
cp: cannot stat `/home/ddahl/code/moz/mozilla-central/mozilla/browser/app/default256.png': No such file or directory
make[2]: *** [export] Error 1
make[2]: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/browser/app'
make[1]: *** [export] Error 2
make[1]: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/browser'
make: *** [default] Error 2
make: Leaving directory `/home/ddahl/code/moz/mozilla-central/obj-i686-pc-linux-gnu-optimize/browser'
Comment 39 David Dahl :ddahl 2009-06-08 13:33:42 PDT
accoring to this push by mconnor, I am doing it all wrong: http://hg.mozilla.org/mozilla-central/rev/abe1b8b03969
Comment 40 David Dahl :ddahl 2009-06-08 15:35:10 PDT
Created attachment 382197 [details] [diff] [review]
nsWindow.cpp changes for linux only
Comment 41 Dão Gottwald [:dao] 2009-06-08 16:07:29 PDT
(In reply to comment #38)
> very strange - I think we need to add a Minefield icon to
> browser/app/default256.png for this to build properly.

Right, if we add sizes, we need unbranded icons in those sizes. http://hg.mozilla.org/mozilla-central/rev/abe1b8b03969 and http://hg.mozilla.org/mozilla-central/rev/be17081e57cf are only the first steps.
Comment 42 Dão Gottwald [:dao] 2009-06-08 16:12:36 PDT
Comment on attachment 382169 [details] [diff] [review]
v 0.4 removed changes to browser/app

This replaces browser/branding/unofficial/content/icon16.png, for instance, but it shouldn't. It should replace other-licenses/branding/firefox/default16.png instead, except that mconnor did that already.
Comment 43 Dão Gottwald [:dao] 2009-06-08 16:16:27 PDT
(In reply to comment #42)
> [...] browser/branding/unofficial/content/icon16.png
> [...] other-licenses/branding/firefox/default16.png

err, working examples:

browser/branding/unofficial/content/icon48.png vs.
other-licenses/branding/firefox/content/icon48.png

browser/branding/unofficial/default16.png vs.
other-licenses/branding/firefox/default16.png
Comment 44 David Dahl :ddahl 2009-06-08 19:26:39 PDT
Created attachment 382224 [details] [diff] [review]
WIP patch - started over

waiting on new icons for minefield / branch 22/24/256.
Comment 45 Michael Ventnor 2009-06-08 19:42:31 PDT
If the 22 and 24 icons are too much trouble, just get rid of them. GTK will downscale the 32 icon which is what these icons are likely to be anyway.

And whenever you change the items in extensions[][] make sure you change the numbers as well (comment 29).
Comment 46 Michael Ventnor 2009-06-08 20:06:34 PDT
Uh, but since the 22 and 24 icons are checked in I suppose we have to use them...

Never mind me...
Comment 47 David Dahl :ddahl 2009-06-08 23:17:48 PDT
No worries. The WIP patch hnadles most of this stuff now. I have been in conversation with mconnor about it all.
Comment 48 David Dahl :ddahl 2009-06-10 09:24:52 PDT
Created attachment 382514 [details] [diff] [review]
v 0.1.1 Linux Icons - Trunk Patch

Trunk patch - added placeholders for browser/app and browser/branding/unofficial
Comment 49 David Dahl :ddahl 2009-06-10 11:01:36 PDT
Created attachment 382536 [details] [diff] [review]
v 0.1.2 Trunk Patch

ran pngcrush on each png, some of the old pngs were corrupt. weird. Borris made new ones
Comment 50 David Dahl :ddahl 2009-06-11 11:33:40 PDT
Created attachment 382779 [details] [diff] [review]
v 0.1 Branch / 191 patch

Just putting up a WIP. Getting build errors related thebes right now: http://pastebin.mozilla.org/656930
Comment 51 Dão Gottwald [:dao] 2009-06-11 12:32:56 PDT
Comment on attachment 382536 [details] [diff] [review]
v 0.1.2 Trunk Patch

> ICON_FILES    = \
>         $(DIST)/branding/mozicon128.png \
>-        $(DIST)/branding/mozicon50.xpm \
>-        $(DIST)/branding/mozicon16.xpm \
>+        $(DIST)/branding/default256.png \
>         $(DIST)/branding/document.png \
>         $(NULL)

Why is default256.png part of that group, rather than together with default16.png, default22.png etc.?

>-    const char extensions[6][7] = { ".png", "16.png", "32.png", "48.png",
>-                                    ".xpm", "16.xpm" };
>+
>+    const char extensions[9][8] = { ".png", "16.png", "32.png", "48.png",
>+                                    "32.png", "48.png", "128.png", 
>+                                    "256.png" };

The numbers are messed up (22, 24, 32, 48), and unless I'm missing something, extensions[9][8] should be extensions[8][8].
Comment 52 Dão Gottwald [:dao] 2009-06-11 12:37:59 PDT
Comment on attachment 382779 [details] [diff] [review]
v 0.1 Branch / 191 patch

>+    const char extensions[9][8] = { ".png", "16.png", "22.png", "24.png",
>+                                    "32.png", "48.png", "256.png", 
>                                     ".xpm", "16.xpm" };

128.png isn't missing intentionally, is it?
Comment 53 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-11 18:03:32 PDT
This bug has morphed to adding support for just the additional resolutions, and Linux will now pick up the new icon at the current resolutions, right?
Comment 54 David Dahl :ddahl 2009-06-12 08:42:32 PDT
in response to comment 52: Seems like 128.png was never in the extensions array. Weird. I'll fix it now.

in response to comment 53: It should - are you seeing something else in your testing?
Comment 55 David Dahl :ddahl 2009-06-12 08:58:14 PDT
Created attachment 382965 [details] [diff] [review]
v 0.2 Trunk Patch
Comment 56 David Dahl :ddahl 2009-06-12 08:59:41 PDT
Created attachment 382966 [details] [diff] [review]
v 0.2 Branch / 191 Patch
Comment 57 Dão Gottwald [:dao] 2009-06-12 11:34:09 PDT
Comment on attachment 382965 [details] [diff] [review]
v 0.2 Trunk Patch

> ifneq (,$(filter gtk2,$(MOZ_WIDGET_TOOLKIT)))
>     cp $(srcdir)/mozicon128.png $(DIST)/branding/mozicon128.png
>-    cp $(srcdir)/mozicon16.xpm $(DIST)/branding/mozicon16.xpm
>-    cp $(srcdir)/mozicon50.xpm $(DIST)/branding/mozicon50.xpm
>+    cp $(srcdir)/default256.png $(DIST)/branding/default256.png
>     cp $(srcdir)/document.png $(DIST)/branding/document.png
> endif
> ifeq ($(MOZ_WIDGET_TOOLKIT),gtk2)
>     cp $(srcdir)/default16.png   $(DIST)/branding/default16.png
>+    cp $(srcdir)/default22.png   $(DIST)/branding/default22.png
>+    cp $(srcdir)/default24.png   $(DIST)/branding/default24.png
>     cp $(srcdir)/default32.png   $(DIST)/branding/default32.png
>     cp $(srcdir)/default48.png   $(DIST)/branding/default48.png

default256.png should come right after default48.png.

> BROWSER_APP_FILES = \
>     default16.png \
>+    default22.png \
>+    default24.png \
>     default32.png \
>     default48.png \
>     mozicon128.png \
>-    mozicon16.xpm \
>-    mozicon50.xpm \
>+    default256.png \
>     firefox.ico \
>     document.ico \
>     $(NULL)

here too
Comment 58 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-12 16:04:59 PDT
>in response to comment 53: It should - are you seeing something else in your
>testing?

Just making absolutely sure, if we didn't get that part in before freeze I would have villagers coming after me with torches and pitchforks.
Comment 59 David Dahl :ddahl 2009-06-12 16:16:59 PDT
Alex:

Actually, mconnor is doing some checking with distro contacts before we proceed, as this is more involved than I realized.
Comment 60 Dão Gottwald [:dao] 2009-06-12 23:22:42 PDT
Distros with custom branding will have to add 22/24/264px versions of their icon or undo part of this patch... Am I missing something else?
Comment 61 Mike Connor [:mconnor] 2009-06-13 09:25:26 PDT
Unless I'm on crack, if there's no match, the code handles that fine (if you dig way way down deep).
Comment 62 Dão Gottwald [:dao] 2009-06-13 09:57:21 PDT
You'll get browser/app/default22.png if the branded default22.png is missing, won't you?

By the way, it seems like browser/branding/unofficial/Makefile.in also needs an update...
Comment 63 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-15 12:52:22 PDT
This image was posted to my blog, not sure what is causing the title bar icon to be cropped: http://i43.tinypic.com/2aj9lp0.jpg
Comment 64 David Dahl :ddahl 2009-06-16 12:20:17 PDT
re: comment 63  - I assume that is a linux/theme issue. I have a similar looking Gnome theme and the icon looks normal.
Comment 65 Dão Gottwald [:dao] 2009-06-16 12:27:28 PDT
I can't reproduce that either.
Comment 66 Alex Faaborg [:faaborg] (Firefox UX) 2009-06-18 16:19:10 PDT
Follow up bug to remove some icons previously used on linux: bug 499226
Comment 67 Samuel Sidler (old account; do not CC) 2009-07-23 16:25:08 PDT
Is there more left to do here for 1.9.1 or is this bug fixed?
Comment 68 David Dahl :ddahl 2009-07-23 16:29:44 PDT
I think this is fixed. I believe mconnor checked in the correct icons - the only followup that might make sense is removing any xpm linux icons and associated code that is deprecated - which should be a separate bug.
Comment 69 Samuel Sidler (old account; do not CC) 2009-07-23 16:56:27 PDT
Please re-nom for status1.9.1 if this turns out not to be fixed.

Note You need to log in before you can comment on or make changes to this bug.