User-Agent: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20110327 Firefox/4.0 Iceweasel/4.0 Build Identifier: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20110327 Firefox/4.0 Iceweasel/4.0 Firefox 4 moved away from using defaultFavicon.png for Linux, instead opting for moz-icon://stock/gtk-file via CSS. Panorama's use of defaultFavicon.png on app tabs breaks that consistency. Reproducible: Always Steps to Reproduce: 1.Open a tab that uses default favicon. 2.Pin as app tab 3.Open panorama view Actual Results: See the old defaultFavicon.png Expected Results: See the default favicon as used elsewhere in the browser. In order to fix this, the app tabs need to be moved to using CSS for fallback when a custom favicon isn't there. CSS allows different values to be applied by platform, which is how the problem is handled elsewhere.

I've left the definition that points to the defaultFavicon.png in tabview/modules/utils.jsm, as it is still used in tabview/tabitems.js:_update, which likely needs fixing as well.

Please ask for review from one of the browser peers. https://developer.mozilla.org/En/Developer_Guide/How_to_Submit_a_Patch#Getting_Reviews

Includes previous changes, but finishes the job of removing the script-based default favicon code from panorama.

Comment on attachment 525143 [details] [diff] [review] Adds styles to use default favicons, removes script setting the default favicon. Let's have ttaubert do feedback before getting a review.

Thanks for your patch, Adam! When reviewing it I thought about what would be the best solution for this problem and I think we shouldn't fix the symptoms but rather the source of the error. So this should depend on bug 660206 and then we're done with a tiny patch. Sorry for stealing this but the patch is so small I just though I'd attach it :)

Oops, now with the correct favicon service variable.

Comment on attachment 535606 [details] [diff] [review] patch v2 Please check browser_tabview_bug600645.js. It's using contentWindow.Utils.defaultFaviconURL which is removed in this patch.

(In reply to comment #7) > Please check browser_tabview_bug600645.js. It's using > contentWindow.Utils.defaultFaviconURL which is removed in this patch. You're right I should've run the tests before asking for feedback, my bad.

Comment on attachment 535609 [details] [diff] [review] patch v3 Looks good!

Comment on attachment 535609 [details] [diff] [review] patch v3 Trying to distribute reviews, hope Shawn has some time left :)

Comment on attachment 535609 [details] [diff] [review] patch v3 Review of attachment 535609 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh ::: browser/base/content/tabview/groupitems.js @@ +2031,5 @@ > > if (UI.shouldLoadFavIcon(xulTab.linkedBrowser)) > iconUrl = UI.getFavIconUrlForTab(xulTab); > else > + iconUrl = gFavIconService.defaultFavicon.spec; Linux should use moz-icon://stock/gtk-file, but I see you are trying to fix that in general in bug 660206, so I'm fine with landing this like this as now (it's no worse than the current situation).

Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0 Panorama's use of the default favicon is still inconsistent on Linux. The problem can be reproduced on Firefox7 beta1 with the same steps as in the description. Is this issue known and intended for the Firefox 7 release? For Firefox 8, the default favicon is updated, as specified in Bug 648668.

(In reply to Virgil Dicu from comment #14) > Panorama's use of the default favicon is still inconsistent on Linux. The > problem can be reproduced on Firefox7 beta1 with the same steps as in the > description. Indeed. > Is this issue known and intended for the Firefox 7 release? It is known but will not be fixed in Fx 7. > For Firefox 8, the default favicon is updated, as specified in Bug 648668. Yep, will be fixed in Fx 8.