Last Comment Bug 621091 - Linux Theme should use moz-icon://stock/gtk-file for blank documents
: Linux Theme should use moz-icon://stock/gtk-file for blank documents
Status: RESOLVED FIXED
[good first bug]
:
Product: Firefox
Classification: Client Software
Component: Theme (show other bugs)
: Trunk
: x86 Linux
: -- enhancement (vote)
: Firefox 4.0b10
Assigned To: Pascal Chevrel:pascalc(PTO until Sept 2)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-12-22 19:49 PST by Pascal Chevrel:pascalc(PTO until Sept 2)
Modified: 2011-01-18 03:29 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
before/after screenshot (48.16 KB, image/png)
2010-12-22 19:49 PST, Pascal Chevrel:pascalc(PTO until Sept 2)
no flags Details
patch (2.08 KB, patch)
2010-12-29 07:41 PST, Pascal Chevrel:pascalc(PTO until Sept 2)
no flags Details | Diff | Splinter Review
updated patvch (4.81 KB, patch)
2011-01-03 07:07 PST, Pascal Chevrel:pascalc(PTO until Sept 2)
dao+bmo: review+
Details | Diff | Splinter Review
updated patch (removed 2 empty lines) (4.81 KB, patch)
2011-01-03 08:25 PST, Pascal Chevrel:pascalc(PTO until Sept 2)
pascalc: review+
mbeltzner: approval2.0+
Details | Diff | Splinter Review

Description Pascal Chevrel:pascalc(PTO until Sept 2) 2010-12-22 19:49:55 PST
Created attachment 499462 [details]
before/after screenshot

The Linux theme uses a png for blank document (chrome://global/skin/icons/folder-item.png) that looks a bit old and not very crisp, IMO it should be replaced by the standard gtk blank document which conveys better the concept of "blank, default" than the current icon and looks both more modern and less 'busy'.

The stock gtk icon is also almost identical to what we use for Windows XP and it is the icon used by Chrome on Linux.

I am attaching a screenshot before/after of this proposal.
Comment 1 Pascal Chevrel:pascalc(PTO until Sept 2) 2010-12-29 07:41:32 PST
Created attachment 500195 [details] [diff] [review]
patch

Here is a patch fixing this issue, there is one context in which the older icon is used, panorama mode on the top left of thunbnails, but I couldn't find where this was defined.
Comment 2 Dão Gottwald [:dao] 2011-01-02 06:05:08 PST
Comment on attachment 500195 [details] [diff] [review]
patch

This makes folder-item.png unused in gnomestripe, doesn't it? In that case that file should be removed.

We should probably use size=menu in these cases at least:

> /* Bookmark items */
> .bookmark-item:not([container])  {
>-  list-style-image: url("chrome://global/skin/icons/folder-item.png");
>-  -moz-image-region: rect(0px, 16px, 16px, 0px)
>+  list-style-image: url("moz-icon://stock/gtk-file?size=toolbarsmall");
> }

> /* All tabs menupopup */
> .alltabs-item > .menu-iconic-left > .menu-iconic-icon {
>-  list-style-image: url("chrome://global/skin/icons/folder-item.png");
>-  -moz-image-region: rect(0px, 16px, 16px, 0px);
>+  list-style-image: url("moz-icon://stock/gtk-file?size=toolbarsmall");
> }
Comment 3 Dão Gottwald [:dao] 2011-01-02 06:07:34 PST
(In reply to comment #1)
> Created attachment 500195 [details] [diff] [review]
> patch
> 
> Here is a patch fixing this issue, there is one context in which the older icon
> is used, panorama mode on the top left of thunbnails, but I couldn't find where
> this was defined.

This is chrome://mozapps/skin/places/defaultFavicon.png, as defined in nsIFaviconService.idl. That file is used in gnomestripe/browser/places/places.css and gnomestripe/browser/aboutSessionRestore.css as well.
Comment 4 Pascal Chevrel:pascalc(PTO until Sept 2) 2011-01-02 13:39:38 PST
(In reply to comment #2)
> Comment on attachment 500195 [details] [diff] [review]
> patch
> 
> This makes folder-item.png unused in gnomestripe, doesn't it? In that case that
> file should be removed.

The file is in hg/src/toolkit/themes/gnomestripe/global/icons 

is it ok to remove that icon that is in toolkit/ and not in browser/ ? Maybe it will impact applications such as Thunderbird or Seamonkey?

> 
> We should probably use size=menu in these cases at least:
> 

Is there any documentation somewhere about what parameters exists and how to use them? I couldn't find anything in MDC and Google, therefore I am mimicking what I see in other files.
Comment 5 Dão Gottwald [:dao] 2011-01-02 14:39:56 PST
(In reply to comment #4)
> The file is in hg/src/toolkit/themes/gnomestripe/global/icons 
> 
> is it ok to remove that icon that is in toolkit/ and not in browser/ ?

Yes.

> Maybe it will impact applications such as Thunderbird or Seamonkey?

Maybe. If so, they should start using the stock icon as well or, if there's a reason not to do so, package folder-item.png themselves.

> Is there any documentation somewhere about what parameters exists and how to
> use them? I couldn't find anything in MDC and Google, therefore I am mimicking
> what I see in other files.

nsIIconURI.idl lists the parameters:
http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/icon/nsIIconURI.idl#63
Comment 6 Pascal Chevrel:pascalc(PTO until Sept 2) 2011-01-03 02:23:44 PST
The default icon in panorama mode is actually used as an inline <img> here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#1019

and is defined here:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/modules/utils.jsm#480

I didn't find a syntax that would allow me to define the use of a stock gtk icon instead of an image  file in the jar manifest file, the manifest file currently has:

+ skin/classic/mozapps/places/defaultFavicon.png  (places/defaultFavicon.png)

(replacing the relative url places/defaultFavicon.png  by moz-icon://stock/gtk-file?size=menu does not work)
Comment 7 Dão Gottwald [:dao] 2011-01-03 02:46:57 PST
It can be done like this:

   defaultFaviconURL:
#ifdef MOZ_WIDGET_GTK2
     "moz-icon://stock/gtk-file?size=toolbarsmall",
#else
     "chrome://mozapps/skin/places/defaultFavicon.png",
#endif

However, I don't think utils.jsm is currently being preprocessed... So that would need to be changed first, or you could spin it off to a separate bug.
Comment 8 Pascal Chevrel:pascalc(PTO until Sept 2) 2011-01-03 04:56:22 PST
I think that I will deal with the general case in this bug and the panorama one in a separate bug.
Comment 9 Pascal Chevrel:pascalc(PTO until Sept 2) 2011-01-03 07:07:41 PST
Created attachment 500801 [details] [diff] [review]
updated patvch

Here is a new patch adressing the following issues:
- replaces all instances of the png icon by the stock gtk icon, sets opacity for disabled state when needed
- removes the now unused png icon and removes it from the jar manifest
- adds a #ifdef in nsIFaviconService.idl to use the stock icon as well
- adresses Dao's comment about the use of the menu attribute

This patch does not address the panorama case since no #ifdef can be used in this file yet, I will file a separate bug for that.

I compiled locally and everything works for me.
Comment 10 Ludovic Hirlimann [:Usul] 2011-01-03 07:18:09 PST
Andreas if this lands we'll need something for TB.
Comment 11 Dão Gottwald [:dao] 2011-01-03 08:15:35 PST
Comment on attachment 500801 [details] [diff] [review]
updated patvch

>--- a/toolkit/components/places/public/nsIFaviconService.idl
>+++ b/toolkit/components/places/public/nsIFaviconService.idl
>@@ -357,7 +357,15 @@
>  * Notification sent when all favicons are expired.
>  */
> #define NS_PLACES_FAVICONS_EXPIRED_TOPIC_ID "places-favicons-expired"
>+
>+#ifdef MOZ_WIDGET_GTK2
>+#define FAVICON_DEFAULT_URL "moz-icon://stock/gtk-file?size=toolbarsmall"
>+#else
> #define FAVICON_DEFAULT_URL "chrome://mozapps/skin/places/defaultFavicon.png"
>+#endif
>+
> #define FAVICON_ERRORPAGE_URL "chrome://global/skin/icons/warning-16.png"
> 
> %}
>+
>+

nit: don't add these two lines at the bottom
Comment 12 Pascal Chevrel:pascalc(PTO until Sept 2) 2011-01-03 08:25:31 PST
Created attachment 500824 [details] [diff] [review]
updated patch (removed 2 empty lines)

here is the same patch without the 2 extra line to the file
Comment 13 mcdavis941 (sporadically reading bugmail) 2011-01-03 09:27:57 PST
If I'm reading this correctly, then when on GTK2:

- default favicon will be defined to use moz-icon://
- panorama code will get that value from the favicon service when asking what favicon to show for a tab when in panorama view (for tabs representing documents which don't define their own favicons)
- panorama code will set that value as explicit inline img src attribute
- pure CSS will be unable to apply a favicon because CSS list-style-image doesn't apply when img src is set
- when using an em:type=4 theme, the icon will come from the desktop rather than (unlike before this patch) from the theme
- leaving a high potential for mismatch between the style of the icon and the rest of the UI in that case
- and, the same thing will be true for all favicons gotten from the favicon service when on GTK2, i.e., for main browser tabs

Is that right?  And if so, could this be done in a way that preserves the current skinnability?  Like, if the favicon service tells you to fall back to the default favicon, then use in-theme CSS (chrome://browser/skin/tabview/tabview.css) to set it, allowing third-party themes to set the favicons they need?
Comment 14 Pascal Chevrel:pascalc(PTO until Sept 2) 2011-01-03 09:44:36 PST
@mcdavis
The patch in this bug does not affect panorama, I will file a separate bug to use the GTK default icon in panorama (and only the default icon, nothing else).

The moz-icon:// protocol allows setting a specific size in pixels (?size=16), not only proportional ones.
Comment 15 mcdavis941 (sporadically reading bugmail) 2011-01-03 10:00:58 PST
That bit I said about main browser tabs is wrong.  When there's no favicon, theme rules apply.
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-17 21:32:24 PST
Comment on attachment 500824 [details] [diff] [review]
updated patch (removed 2 empty lines)

a=beltzner

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