The work in bug 511240 laid the foundation for processing web page updates/changes and at the same time picked up most of the work for fav/site icons on content tabs. We should pick up the remainder of that work and have nice web-site supplied icons.

Work in progress fix. This is mostly working - using favicon.ico on a site works, as does generating an icon when displaying just a png in a tab. I've not checked if support for the link option works or not yet. There's also some integration into the favicon service in places, though I need to check for other integration points that I may have missed.

This fixes the <link> element stuff so that pages with the favicon specified in those elements will now work (e.g. add-on manager). Still got the favicon service to look over and a bunch of clean up (and tests) to do.

This now has everything working, plus a few small tests thrown in. Most of the code is based on parts that are in Firefox's browser.js and tabbrowser.xml - the changes in tabProgressListener and DOMLinkHandler are the significant blocks copied and adjusted. I've also picked up Firefox's method of having a separate xul:image for the throbber. This makes it much easier to set the favicon on the xul:image whilst still loading the web page.

Comment on attachment 508750 [details] [diff] [review] The fix I think this is more in Andrew's area of expertise… Thanks, Blake.

Comment on attachment 508750 [details] [diff] [review] The fix http://reviews.visophyte.org/r/508750/ on file: mail/base/content/specialTabs.js line 177 > let iconAdded = false; > let relStrings = rel.split(/\s+/); > let rels = {}; > for (let i = 0; i < relStrings.length; i++) > rels[relStrings[i]] = true; > > for (let relVal in rels) { > if (relVal == "icon") { > if (!iconAdded) { I don't understand the point of having a loop here if you are storing the results in an object as a dictionary. Wouldn't rel.split(/\s+/).indexOf("icon") != -1 accomplish the same thing? on file: mail/base/content/specialTabs.js line 207 > let ssm = Components.classes["@mozilla.org/scriptsecuritymanager;1"] > .getService(Components.interfaces.nsIScriptSecurityManager); > > try { > ssm.checkLoadURIWithPrincipal(targetDoc.nodePrincipal, uri, > Components.interfaces.nsIScriptSecurityManager.DISALLOW_SCRIPT); > } Please clarify what is happening here with a comment. My first instinct without consulting the docs is that we want to avoid putting a "javascript:EVIL();" into chrome-space. When I check the docs, this also seems to see what it means. However, I also know (and verify) that "src" is "neither paged not scripted". Is this just a paranoia / hygiene thing? Cargo-culted from Firefox? on file: mail/base/content/specialTabs.js line 221 > try { > var contentPolicy = Components.classes["@mozilla.org/layout/content-policy;1"] > .getService(nsIContentPolicy); > } nit: indent is off by one, but I don't actually care. on file: mail/base/content/specialTabs.js line 230 > // Security says okay, now ask content policy > if (contentPolicy.shouldLoad(nsIContentPolicy.TYPE_IMAGE, > uri, targetDoc.documentURIObject, > link, link.type, null) != > nsIContentPolicy.ACCEPT) > break; I also would appreciate more rationale here. Is the goal to avoid having a content tab show a message body in it and allow it to leak information via information fetch that is not controlled by our policy? on file: mail/base/content/specialTabs.js line 1187 > catch (e) { dump(e); } Please remove the dump or provide a comment that explains why we would want it. on file: mail/base/content/tabmail.xml line 985 > <!-- > - Sets the tab icon to the specified url, or removes the icon if no > - url is specified. Note that this may override css provided images. > --> Please at least put a teeny note about this in the horrible giant documentation blob at the top. on file: mail/themes/gnomestripe/mail/tabmail.css line 141 > .tab-throbber { > list-style-image: url("chrome://global/skin/icons/loading_16.png") !important; > -moz-image-region: auto !important; > } We're confident the display:none stops the animation subsystem from doing something dumb in the background all the time, yes?

(In reply to comment #5) > I don't understand the point of having a loop here if you are storing the > results in an object as a dictionary. Wouldn't > rel.split(/\s+/).indexOf("icon") != -1 accomplish the same thing? I suspect Firefox was trying to do something clever, in any case I don't think we need it, so I've simplified it. > Please clarify what is happening here with a comment. My first instinct > without consulting the docs is that we want to avoid putting a > "javascript:EVIL();" into chrome-space. When I check the docs, this also > seems to see what it means. However, I also know (and verify) that "src" is > "neither paged not scripted". Is this just a paranoia / hygiene thing? > Cargo-culted from Firefox? It seems to go all the way back to bug 191839... From your reasoning it does seem to be just paranoia, but it is copied from Firefox. I think I'd like to keep it in just to keep it the same, so I've added a comment to that effect. > on file: mail/base/content/specialTabs.js line 230 > > // Security says okay, now ask content policy > > I also would appreciate more rationale here. Is the goal to avoid having a > content tab show a message body in it and allow it to leak information via > information fetch that is not controlled by our policy? Bug 191839 comment 93 indicates we may have once loaded data direct from a cache or something. Whether or not that is still true I'm not sure (I kinda doubt it), but I'd prefer to keep the content policy check in there to be safe. > on file: mail/base/content/specialTabs.js line 1187 > > catch (e) { dump(e); } > > Please remove the dump or provide a comment that explains why we would want > it. Oops that was some debug I'd left in. > We're confident the display:none stops the animation subsystem from doing > something dumb in the background all the time, yes? Well, if it wasn't then Firefox with lots of tabs open would be using lots of cpu as they do the same thing. Bug 604557 also indicates they care about that sort of thing (although that's on svg).

Checked in: http://hg.mozilla.org/comm-central/rev/c51913c44f24