Currently checkLoadURI in tabbrowser.xml restricts loading of favicon for chrome-URIs. We should allow loading of favicons here because normal images doesn't have this restriction. I noticed this bug while working on bug 280190. http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbrowser.xml#716 http://lxr.mozilla.org/seamonkey/source/content/base/src/nsContentUtils.cpp#1821 Patch upcoming...

Nromal images have this restriction because known functionality (eg some remote XUL) would break with it. The fact that they don't have this restriction does NOT make me happy... Is there an actual use case for chrome favicons?

(In reply to comment #3) > Is there an actual use case for chrome favicons? As I mentioned before we want to use a favicon on the new netError.xhtml page.

How about only allowing loading of chrome favicons by a chrome page, then?

(In reply to comment #5) > How about only allowing loading of chrome favicons by a chrome page, then? One possibility, but this wouldn't help us due to netError.xhtml doesn't have chrome permissions since bug 292624 was fixed. Or how we could handle his? But setting a restriction for loading chrome favicons by normal webpages is a good idea. They don't need access.

Can't you get the actual URI loaded from the page's docshell and just look at the scheme?

Comment on attachment 189619 [details] [diff] [review] Enable loading of favicons for xpfe Per comments

(In reply to comment #7) > Can't you get the actual URI loaded from the page's docshell and just look at > the scheme? Sorry, I don't really understand, what I should do. You are only interested to know the current scheme of netError? After the checkin of bug 292624 the scheme was changed to "moz-neterror:page?e=error&u=url&d=desc".

That's the URI that starts to be loaded. I'm talking about the URI as actually loaded... biesi, do we have a way of getting at it in this case?

(In reply to comment #10) > That's the URI that starts to be loaded. I'm talking about the URI as actually > loaded... biesi, do we have a way of getting at it in this case? Ok, I've used DOMi to get the documentURI. I hope this is that one you wanna have... It looks like that one: about:neterror?e=dnsNotFound&u=http%3A//www.not.existent/&c=UTF-8&d=www.not.existent%20could%20not%20be%20found.%20Please%20check%20the%20name%20and%20try%20again.

hmm, I guess docshell.currentURI really is the channel's originalURI. document.documentURI maybe?

For an about: load, document.documentURI will be the originalURI of the channel. The webnavigation's currentURI will generally be the channel's final URI, but for error pages I believe we might force it to be the URI the user originally started to load... In any case, is there a reason to allow chrome favicons for anything but "about:neterror" (as documentURI here)?

(In reply to comment #13) > For an about: load, document.documentURI will be the originalURI of the channel. > The webnavigation's currentURI will generally be the channel's final URI, but > for error pages I believe we might force it to be the URI the user originally > started to load... Ok, I tested the values for the above objects. document.documentURI: about:neterror?e=dnsNotFound... getWebNavigation().currentURI: http://www.not.existent/ > In any case, is there a reason to allow chrome favicons for anything but > "about:neterror" (as documentURI here)? At this time I don't think so. Afaik netError.xhtml will be the only page using a chrome referenced favicon.

I'd be in favor of restricting to that, then.

*** Bug 302797 has been marked as a duplicate of this bug. ***

Comment on attachment 189620 [details] [diff] [review] Enable loading of favicons for toolkit equivalent to the one bz rejected

As summarized this is WFM: chrome pages *can* load chrome favicons and checkLoadURI works fine. Rather than a general bug I think this is specific to the about:neterror? page. This should get fixed in tabbrowser rather than caps.

(In reply to comment #18) > As summarized this is WFM: chrome pages *can* load chrome favicons and > checkLoadURI works fine. Indeed a bad summary. Thanks for adjusting to the real issue. > Rather than a general bug I think this is specific to the about:neterror? page. > This should get fixed in tabbrowser rather than caps. Yes, it seems only to be used for netError.xhtml. Personaly I havn't an idea where to start now.

maybe something like this, but this is completely untested (the reason for the chrome check is that I feared XSS attacks on the error page leading to insertion of another <link rel="icon">, which I suspect does load the favicon. I haven't verified that, but I thought better to be on the safe side...)

(I don't intend to do anything with this patch, someone else will have to take care of it)

Christian's patch is what I was suggesting (good call on checking that the favicon is indeed chrome). If we think this is at all useful we could let all about: URIs set a chrome favicon, but this is a conservative change to fix the narrowly stated problem. the Toolkit tabbrowser would need the same patch.

Comment on attachment 191143 [details] [diff] [review] allow favicons for error pages only r=dveditz I have no idea how important neterror favicons are to 1.8b4. This is not marked blocking (or even nominated). We need bsmedberg's (or mconnor's) r= on this.

Comment on attachment 191143 [details] [diff] [review] allow favicons for error pages only Such a favicon will get pushed all the way up the UI to the URLbar and the bookmarks. Pages without their own icon will not get reset when the page loads succesfully. Maybe it's possible to fake a favicon like we do for image documents.

Checking in toolkit/content/widgets/tabbrowser.xml; /cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v <-- tabbrowser.xml new revision: 1.102; previous revision: 1.101 done Checking in xpfe/global/resources/content/bindings/tabbrowser.xml; /cvsroot/mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml,v <-- tabbrowser.xml new revision: 1.125; previous revision: 1.124 done

I backed out the xpfe patch because of comment 25, which may make this patch not needed at least for xpfe.

(In reply to comment #25) > (From update of attachment 191143 [details] [diff] [review] [edit]) > Such a favicon will get pushed all the way up the UI to the URLbar and the > bookmarks. Pages without their own icon will not get reset when the page loads > succesfully. Maybe it's possible to fake a favicon like we do for image > documents. I can't recognize this problem with my current cvs build of Firefox. Domains without a favicon aren't updated with the netError warning icon. Also the bookmark isn't updated. It still shows me the blank favicon after a complete reload. What I have done: - Put a bookmark without favicon to the PTB - Load this page - Shut down the network - Reload the page after clearing the cache - Tab and location bar show the warning icon but not the bookmark - Click bookmark to refresh it - still blank icon - Reconnect network - Reload page - tab and location bar shows blank favicon

(In reply to comment #28) >(In reply to comment #25) >>Such a favicon will get pushed to the URLbar and bookmarks. >I can't recognize this problem with my current cvs build of Firefox. No, it turns out that Firefox only update the bookmarks icon when a page loads (error pages don't load, they just show). So as long as you don't patch xpfe's tabbrowser.xml then we'll be fine.

note that the xpfe part of bug 229737 is now checked in, so seamonkey shouldn't change any favicons when error pages are loaded.

(In reply to comment #30) > note that the xpfe part of bug 229737 is now checked in, so seamonkey shouldn't > change any favicons when error pages are loaded. So we want to fix this bug or are favicons for xul error pages out of scope for now?

(In reply to comment #31) >So we want to fix this bug The patch still applies, so I guess biesi can check it in (it's his patch)...

can someone else take care of checking this in for me, if it's still needed?

(In reply to comment #33) > can someone else take care of checking this in for me, if it's still needed? You mean backing it out, as per bug 430433?

> You mean backing it out, as per bug 430433? No Biesi means that the patch needs to be landed.

>> You mean backing it out, as per bug 430433? >No Biesi means that the patch needs to be landed. But in /suite/ and not in /xpfe/

The patch needs to updated to apply to the suite tabbrowser, and should probably included about:blocked like the current Firefox code in browser.js does. It should probably also be tested and maybe even reviewed again after someone's done that.

So which patches have to be updated? Both or only the one for suite? Reopening bug until the remaining work is done and an updated patch is checked in.

The suite patch needs to be updated for the tabbrowser.xml living in suite/ - this is probably the cause of bug 431955.

This patch updates the suite tabbrowser.xml in the same way how Firefox handles it.

Comment on attachment 319128 [details] [diff] [review] Updated patch for SeaMonkey I don't really understand tabbrowser code, over to Neil who should actually know it.

Comment on attachment 319128 [details] [diff] [review] Updated patch for SeaMonkey >+ const aboutBlocked = /^about:blocked\?/; We don't (yet?) have about:blocked, so I don't think we should test for it, it will only confuse people. r=me with that fixed. >+ const secMan = >+ Components.classes["@mozilla.org/scriptsecuritymanager;1"] >+ .getService(nsIScriptSecurityManager); Nit: need to reindent all three lines, not just the first ;-)

Fixed comments and taken over r+. Robert, do we need sr for this module?

Given that Neil as our "UI tsar" has done the review, I guess it's ok to go in this way.

Ok, so I need someone to check this in. Thanks.

Checking in suite/browser/tabbrowser.xml; /cvsroot/mozilla/suite/browser/tabbrowser.xml,v <-- tabbrowser.xml new revision: 1.196; previous revision: 1.195 done Is this fixed now? If so, leaving to Henrik to mark it as such :)

Comment on attachment 322405 [details] [diff] [review] Updated patch for SeaMonkey v2 [Checkin: Comment 46] >+ if (!(aboutNeterr.test(targetDoc.documentURI) || >+ !uri.schemeIs("chrome")) { Grrr, the first of those lines has a ( too much, killed the one after the ! as a bustage fix, thanks to ajschult for pointing this out.

Sorry for the bustage. But with the follow-up fix this bug should be resolved now. Marking accordingly.