595462 - Support loading window icons from zips

Status: RESOLVED WONTFIX

(Core :: Widget, defect)

Description

[Michael Wu [:mwu]]

Extensions that aren't unpacked can't easily set their own window icon without extracting it. We should support loading window icons from jars. This is also an issue for Linux omnijar that prevents icons from being a part of the omnijar.

Comment 1

[Benjamin Smedberg]

Dammit, I want <link rel="icon"> to work for XUL windows. Oh well, let's do the minimum amount necessary to get this working.

Comment 2

[Sergey Sokoloff]

Do you plan href="chrome://…", or href="resource://…", or both?

Comment 3

[Nils Maier [:nmaier]]

Attachment: prototype 0, load window icons from everywhere/from all supported formats

I found the time to do some prototyping, and it is actually quite simple to load icons from arbitrary places and formats.
The patch is currently Windows only, but is should be easy to adept to gtk/qt, while mac doesn't use window icons anyway.
I didn't think too much about API just yet, just wanted to see what would be required.

Try out:
* Apply patch and build
* Add windowicon="chrome|resource|file|data url" attribute to a window (before loading)

I'd like to get some feedback/ideas/suggestions on how to tackle the following before I might continue, i.e. I'd like to work out some specs first:
* What would be the best approach to load image lists, i.e. small and large icons, from markup and implementation perspective?  Windows uses small/large icons and gtk/qt support specifying whole icon lists.
So, allow to specify a whole list of icons, incl. hinting which should be the default small and default large for windows, which would require to read multiple Elements (e.g. <link rel="(small|large)?icon">) or just allow to define two icons, small and large which can be done using two distinct Node attributes (e.g <window smallicon="" largeicon="">) or still use Elements (e.g. <link>)
Supporting just a small and large icon is more trivial to implement, but supporting real lists would likely be more compatible with *nix gfx toolkits.

* Not all platforms (i.e. mac) will use window icons. What would be the best approach to not load icons for these platforms? Loading code in the actual platform-specific widget implementation? ifdefs? Exposing a flag in the widget interface?

* Should it be a MUST to support changing icons dynamically? I'd say it is enough to load then just once after the initial chrome load finishes.

Comment 4

[Sergey Sokoloff]

(In reply to comment #3)
> * What would be the best approach to load image lists, i.e. small and large
> icons, from markup and implementation perspective?  Windows uses small/large
> icons and gtk/qt support specifying whole icon lists.
> So, allow to specify a whole list of icons, incl. hinting which should be the
> default small and default large for windows, which would require to read
> multiple Elements (e.g. <link rel="(small|large)?icon">) or just allow to
> define two icons, small and large which can be done using two distinct Node
> attributes (e.g <window smallicon="" largeicon="">) or still use Elements
> (e.g. <link>)

I'd say, take the Web approach: multiple <link rel="icon"> elements.

In the Web, a browser takes the last icon it understands (e.g. if the first is .ico and the second is .gif, then Firefox takes .gif, but IE6 takes .ico, being unable to render .gif icons).

In XUL, the browser should also ignore the unknown icons (for the sake of forward compatibility with future Firefox versions capable to render currently unknown formats). Then the list of known icons becomes an obvious (and probably our best) choice to pass to gtk/qt (as the desired icon list) where gtk/qt support is available.

Under Windows you may try to analyse the sizes and pick the pair of icons most appropriate (provided that the browser knows the actual system metrics of what the "small icon" and the "large icon" is, in the current theme of the OS interface), probably preferring downscaling a larger icon to upscaling a smaller, if the exact desired size isn't available. You may also let the XUL author to decide, e.g. making multiple <link rel="icon large"> and multiple <link rel="icon small"> elements possible.

> * Should it be a MUST to support changing icons dynamically? I'd say it is
> enough to load then just once after the initial chrome load finishes.

Being able to "flash" the window icon (say, replacing it with the "incoming messages available" icon, and then back, each second) is a great method for the background window to signalize that it wants some attention. Thunderbird would probably flap its wings whenever some mail is incoming (as "The Bat!" mail client already does).

Comment 5

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

My main concern here is that the new code does an asynchronous load whereas the old code did a synchronous load. It seems likely that at least under some conditions, this will mean the window appears with the default icon and then changes to the real icon later. That seems bad.

Comment 6

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

I think somehow we're going to have to make the icon load "delay the load event" for the XUL window like other resources do.

Comment 7

[Nils Maier [:nmaier]]

Attachment: patch v1, load window icons from everywhere/from all supported formats

This thing should be (mostly) complete by now:
* Support specifying icons via <link rel="icon" href="abs/rel"/>
  - Gathered using querySelectorAll('link[rel="icon"]')
* (Perf) Removing the obsolete loader code, which, with all that fstat'ing, is bad for performance
  - i.e. custom icons are now loaded on demand (reactive) instead of trying to load icon(s) for each window
* Delay making windows visible until after all custom icons are loaded (:roc)
* Windows: Best-icon selection for BIG/SMALL icons
  - Choose the icon with the preferred dimensions
  - If not available choose the smallest icon that is larger than the preferred dimensions
  - Else choose whatever is available
* Gtk2/Qt: Implementation using real icon lists (GList<GPixmap>, QIcon)
* Windows: (Perf) Remove loading of the default icon as it is already loaded elsewhere
* Gtk2/Qt: (Perf) Use a variation of the old icon loading code
  - but only querying the main chrome dir
  - but still loading the icons synchronous from real files
  - but using a static lazy initializer to only generate the default icon once
  - but not trying to load .xpm files, just pngs, as xpms aren't shipped anymore
* Other: Do not attempt to load icons, as those platform have no support (see WantIcons(), decided against #ifdefs)

Open issues (if you want to call them that) and remarks:
* Qt: Default icons aren't loaded, as the widget is not available when this is attempted. This is not caused by the patch but a preexisting bug.
* Others: I lack the build-envs to build and test there. Somebody donate a try-server, to see if mac/android/whatever unexpectedly goes up in flames?
* OS/2: Disabled the custom icon loading code althoughter, as I lack the build environment and expertise to port the new code there. How to proceed?
* Icons are only loaded once, i.e. there is no DOM mutation listener to dynamically replace icons. As a whole list can/should be specified anyway, dynamic modification would be a bitch anyway.
* Icons aren't scaled to preferred dimensions, hence the native gfx toolkit (win,gtk2,qt) will scale them itself. Maybe the mozilla image scaling code would produce better results?
* Windows: As .ico is now loaded by the usual loader, only one image is extracted - People should replace ico with individual files, as was already the case with *nix
* Firefox does not use custom icons itself IIRC, but Tb and Sm do. Who to CC?

Successfully builds and tests on:
- Win7 x64
- Ubuntu 10.10 x86_64 default toolkit (cairo-gtk2)
- Ubuntu 10.10 x86_64 default toolkit (cairo-qt; ree remark)

Try:
* Build
* Add some <link rel="icon"> elements to a XUL doc of your choosing
* Do not use the html namespace, as the XUL tree-builder does not know html:link is in fact a non-visible element (it will wreck your XUL doc)

Comment 8

[Neil Deakin]

(In reply to comment #4)
> I'd say, take the Web approach: multiple <link rel="icon"> elements.

Since this is a terrible name and syntax for this, I'd rather you did not.

Using a tagname such as <windowicon src=""/> and assuming that it had to be directly inside the <window> would allow you to just iterate over a child list rather than using querySelectorAll which is notably slower as your syntax has to compare an attribute on every element in the document.

> Should it be a MUST to support changing icons dynamically?

This isn't really needed.

> Delay making windows visible until after all custom icons are loaded

Do you mean until after just the icon(s) that are needed are loaded? Do we know which icon(s) are going to end up being used?

> OS/2: Disabled the custom icon loading code althoughter, as I lack the build
environment and expertise to port the new code there. How to proceed?

Usually, we just adjust something if its simple, but let someone else deal with fixing any errors or problems.

Comment 9

[Nils Maier [:nmaier]]

(In reply to comment #8)
> (In reply to comment #4)
> > I'd say, take the Web approach: multiple <link rel="icon"> elements.
> 
> Since this is a terrible name and syntax for this, I'd rather you did not.
> 
> Using a tagname such as <windowicon src=""/> and assuming that it had to be
> directly inside the <window> would allow you to just iterate over a child list
> rather than using querySelectorAll which is notably slower as your syntax has
> to compare an attribute on every element in the document.
> 

Make it <link>, <windowicon> or whatever, I don't really care. But actually I'm more in favor or <link>, simply because it is already widely known and I don't agree on the terrible name. Inventing new syntax for every new thing is far more "terrible" IMO.

As for QuerySelectorAll: Really that much slower? I doubt that. Just look at the css rules that each and every document, XUL or not, has applied by default. Or all the iterating, drawing and what else goes on. I hence doubt any implementation does matter a great deal.
But then again QSA already uses an optimized, deCOMterminated implementation internally, so it might as well be faster than the iterate-approach you're suggesting, at least for the usually quite small trees of XUL documents.

> > Delay making windows visible until after all custom icons are loaded
> 
> Do you mean until after just the icon(s) that are needed are loaded? Do we know
> which icon(s) are going to end up being used?

Until all icons are loaded, because you cannot know before which will be used. Actually you don't know what icons will be used afterwards, because at least Gtk2/Qt are free to use whatever icon they deem is most appropriate.
So in a way all icons are "needed". And you cannot know the icon dimensions before so pre-selecting icons to load isn't a feasible perf optimization.
So all that icon loading/image decoding might add a small perf hit for windows using a lot of custom icons, but usually you should get a perf win because all that iterating over directory contents and stat'ing of the old code is removed and on *nix all found icons would have been loaded/decoded anyway.
And you get a clear perf win for all windows that don't have custom icons specified.
 
> > OS/2: Disabled the custom icon loading code althoughter, as I lack the build
> environment and expertise to port the new code there. How to proceed?
> 
> Usually, we just adjust something if its simple, but let someone else deal with
> fixing any errors or problems.

Well, I commented out the broken code and let a XXX with a reference to this bug.

Comment 10

[Neil Deakin]

(In reply to comment #9)
> Make it <link>, <windowicon> or whatever, I don't really care. But actually I'm
> more in favor or <link>, simply because it is already widely known and I don't
> agree on the terrible name. Inventing new syntax for every new thing is far
> more "terrible" IMO.

Window icons don't have an existing markup syntax.


> As for QuerySelectorAll: Really that much slower? I doubt that. Just look at
> the css rules that each and every document, XUL or not, has applied by default.

We don't iterate over the entire tree for every selector on load though.


> Or all the iterating, drawing and what else goes on. I hence doubt any
> implementation does matter a great deal.
> But then again QSA already uses an optimized, deCOMterminated implementation
> internally, so it might as well be faster than the iterate-approach you're
> suggesting, at least for the usually quite small trees of XUL documents.

I'm suggesting you iterate using nsIContent which also doesn't use xpcom references and so forth either.

Comment 11

[Sergey Sokoloff]

Additional note:

as it seems, the logic for <link rel="icon"> has been extended in HTML5 (section 4.12.4.7) and now provides an additional attribute (sizes="…") in order to declare the list of available sizes in each of the defined icons:

http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon

Example code from there:

  <link rel=icon href=favicon.png sizes="16x16" type="image/png">
  <link rel=icon href=windows.ico sizes="32x32 48x48" type="image/vnd.microsoft.icon">
  <link rel=icon href=mac.icns sizes="128x128 512x512 8192x8192 32768x32768">
  <link rel=icon href=iphone.png sizes="57x57" type="image/png">
  <link rel=icon href=gnome.svg sizes="any" type="image/svg+xml">

Looks pretty cool and should probably be adopted in XUL as is.

Comment 12

[tomorrow]

Any progress on this?
Im unable to set custom menu icons pointing to extensions packed in .xpi due to this bug.

Comment 13

[Jim Mathies [:jimm]]

We'll never get to this.