------- Bug 304561 Comment #16 From Ian 'Hixie' Hickson ------- Is there any reason we can't do what you suggested above, vlad, and "simply" generate a resized version of the image and set that as the favicon instead of resizing the big one each time? We could also use that image as the favicon when you bookmark an image, if we did that, without putting multi-megabyte images into the bookmarks file. ------- Bug 304561 #17 From Vladimir Vukicevic ------- That sounds great; let me know how I generate a resized version of the image and set it as the src for an <img> :) ------- Bug 304561 Comment #18 From Ian 'Hixie' Hickson ------- Well, one inefficient way would be to draw the image to a canvas and then grab a data: URI out of the canvas... But I'm not suggesting doing it that way. Can't imagelib provide some sort of convenient API to do something like this? ------- Bug 304561 Comment #19 From Vladimir Vukicevic ------- Actually, that would be a pretty efficient way; the problem is that we have no way of encoding an image back into data for creating that data URI. We'll have something in place for gecko1.9/future, but not for ffox 1.5.

Hijacking this bug for SeaMonkey too...

This preserves the ability to fall back to the old code in non-canvas-enabled builds.

Same deal as the SeaMonkey patch, but untested as I don't build Firefox. Can someone test this before I request a review? If it doesn't work, it's most likely + var tabImg = document.getAnonymousNodes(this.mTab)[0].firstChild; which needs to get the actual image in the tab.

Comment on attachment 226751 [details] [diff] [review] SeaMonkey patch >+ var tabImg = document.getAnonymousNodes(this.mTab)[0].firstChild; This makes unrealistic assumptions about the tab bindings. >+ var theStyle = window.getComputedStyle(tabImg, null); >+ var w = parseInt(theStyle.width); >+ var h = parseInt(theStyle.height); >+ canvas.width = w; >+ canvas.height = h; IMHO you should use .boxObject.width/height. >+ if (req && req.image.width <= sz && req.image.height <= sz) You should always verify the request of an image document.

More robust tab-image-finding, use boxObject dimensions, verify the image request didn't fail before using it.

Comment on attachment 232625 [details] [diff] [review] SM patch, v2 >+ var tabImg = this.mTab.getElementsByTagName("img")[0]; I don't think this will work at all, because the image is anonymous. It's not the only option, but maybe you meant to combine this with your first version. Note that in the them I'm thinking of, the tab binding looks like this: <content> <xul:box class="tab-outer-box" flex="1"> <xul:box class="tab-middle-box" flex="1"> <xul:box class="tab-inner-box" flex="1" align="center"> <xul:image class="tab-image" xbl:inherits="busy,src=image,validate"/> <xul:box class="tab-focus-box" flex="1"> <xul:text flex="1" xbl:inherits="value=label,crop,accesskey" crop="right" class="tab-text"/> <children/> </xul:box> <children includes="image"/> </xul:box> </xul:box> </xul:box> </content> While I'm prepared to adapt the binding to work with tab thumbnails, document.getAnonymousNodes(this.mTab)[0].firstChild has no hope of working.

(In reply to comment #6) > (From update of attachment 232625 [details] [diff] [review] [edit]) > >+ var tabImg = this.mTab.getElementsByTagName("img")[0]; > I don't think this will work at all, because the image is anonymous. It's not > the only option, but maybe you meant to combine this with your first version. I must not have recompiled when I tested it - as you said, it doesn't work at all. Let's go with: var tabImg = this.mTab.getAnonymousElementsByAttribute("class", "tab-image"); ...which does work.

(In reply to comment #7) >var tabImg = this.mTab.getAnonymousElementsByAttribute("class", "tab-image"); I'd prefer you set an anonid attribute, in case we need to change the class.

Comment on attachment 232625 [details] [diff] [review] SM patch, v2 As per previous comments.

Fixed in SeaMonkey.

Comment on attachment 238569 [details] [diff] [review] Firefox patch As I noted on IRC, globalBindings.xml of each theme overrides this too. On current trunk, only Pinstripe has an extended binding, but as soon as the new theme lands on trunk, this won't affect winstipre as well. Also, you may want to replace the try..catch blocks with #ifdef MOZ_ENABLE_CANVAS.

Fixes pinstripe. I prefer try/catch to #ifdef.

Comment on attachment 238828 [details] [diff] [review] firefox patch Ooops.

Comment on attachment 238829 [details] [diff] [review] firefox patch Please do #ifdef this for toolkit, also, you need to patch the updated overriding bindings of winstripe and pinstripe.

Someone else can take care of the Firefox version.

I'm going to declare this FIXED for SM. If someone wants to port bits to firefox, please file a new bug.