Closed
Bug 719461
Opened 12 years ago
Closed 12 years ago
iconchange event for <iframe browser>
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: benfrancis, Assigned: daleharvey)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 6 obsolete files)
8.93 KB,
patch
|
Details | Diff | Splinter Review |
In the style of the existing locationchange, loadstart and loadend events (https://bugzilla.mozilla.org/show_bug.cgi?id=710231), we also need an event for iconchange, which fires when the favicon URL of the HTML document in an iframe changes. The event object should include the URL as a string.
Comment 1•12 years ago
|
||
What we use in fennec is the DOMLinkAdded event, which is a bit more generic (it covers all <link> tags). I think this would be better also for b2g/gaia
Reporter | ||
Comment 2•12 years ago
|
||
Fabrice, does that get triggered when that link *changes* as opposed to just being added into the document? Sometimes web apps change the favicon as a visual notification.
Comment 3•12 years ago
|
||
> What we use in fennec is the DOMLinkAdded event, which is a bit more generic (it covers all <link>
> tags).
What other <link> elements do you care about?
Comment 4•12 years ago
|
||
Note that I'm tempted to send along the favicon itself in the event, in which case a generic link-added event wouldn't be so helpful.
Comment 5•12 years ago
|
||
(In reply to Justin Lebar [:jlebar] from comment #3) > > What we use in fennec is the DOMLinkAdded event, which is a bit more generic (it covers all <link> > > tags). > > What other <link> elements do you care about? There's mainly rel=alternate to refer to RSS/atom feeds, and rel=search for search engines. Not a huge priority for b2g for sure.
Updated•12 years ago
|
Version: unspecified → Trunk
Comment 6•12 years ago
|
||
Wouldn't that cost a bit too much?
Comment 7•12 years ago
|
||
(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #6) > Wouldn't that cost a bit too much? Sending the favicon, say base-64 encoded? The mozilla.org favicon, e.g., is 3445 characters in base64. I dunno if that's too big!
Reporter | ||
Comment 8•12 years ago
|
||
I'd be happy with a URL if that's any better, encoding/decoding takes time. But judging by the fact that Mounir made this comment on the titlechange event too, I don't think it was the size of the event object he was worried about?
Comment 9•12 years ago
|
||
(In reply to Ben Francis from comment #8) > I'd be happy with a URL if that's any better, encoding/decoding takes time. > > But judging by the fact that Mounir made this comment on the titlechange > event too, I don't think it was the size of the event object he was worried > about? The simple fact that we have to watch for changes in HTMLDocumentElement/HTMLIFrameElement worries me. I wonder if having a <browser> element wouldn't make sense? So this new code wouldn't pollute more common use cases.
Comment 10•12 years ago
|
||
If you're concerned about slowing down normal <iframe>s, we don't need to watch anything if the iframe doesn't have the mozbrowser attribute. Does that address your concerns?
Comment 11•12 years ago
|
||
> Fabrice, does that get triggered when that link *changes* as opposed to just being added into the
> document?
Turns out, we don't recognize favicon changes unless a new link element is added to the document.
Which is kind of lame.
Comment 12•12 years ago
|
||
No other browser handles changing the <link rel='icon'>'s href either, so I guess we're in the clear.
Reporter | ||
Comment 13•12 years ago
|
||
Huh. Now I come to think of it I think all that browsers do is highlight the favicon if the <title> element changes. Seems like an obvious feature to update the favicon, but if nobody uses it it's hardly a priority! So what do you propose? That we have a DOMLinkAdded event like Fabrice suggested?
Comment 14•12 years ago
|
||
(In reply to Ben Francis from comment #13) > So what do you propose? That we have a DOMLinkAdded event like Fabrice > suggested? We can't access the inner frame's DOM anyway, so DOMLinkAdded won't be much help. And anyway, DOMLinkAdded is the wrong abstraction; the browser shouldn't care when the page adds a <link rel=stylesheet>. So I think the right thing is an iconchange event, which has the favicon's URL. It'll be triggered by DOMLinkAdded, but that's an implementation detail.
Reporter | ||
Comment 15•12 years ago
|
||
Sounds good to me.
Assignee | ||
Comment 16•12 years ago
|
||
It isnt just DomLinkAdded, favicons can be dynamically set with data uri's http://www.p01.org/releases/DEFENDER_of_the_favicon/ I will work on an iconchange even based on DOMLinkAdded, and see how problematic observing the last link is at the same time
Assignee | ||
Comment 17•12 years ago
|
||
Also worth remembering that a lot of sites dont have a favicon link, they just expect the browser to automatically check at root
Reporter | ||
Comment 18•12 years ago
|
||
Who would have thought favicons could be so complicated? I guess you could try loading the favicon from the de-facto standard location on locationchange and then set/reset it on iconchange.
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #621062 -
Flags: review?(21)
Assignee | ||
Comment 20•12 years ago
|
||
That patch works fine for setting favicons programatically, it doesnt care about when the page doesnt provide a link to the favicon, it also doesnt care about multiple favicons, those are both left up to the browser. http://www.w3.org/2005/10/howto-favicon
Comment 21•12 years ago
|
||
Comment on attachment 621062 [details] [diff] [review] Add the iconchange event Thanks for the patch! You'll need to add a test for this. See dom/tests/mochitest/test_browserFrame*.html > if (/icon/.test(e.target.rel)) { So if the rel is "fooiconbar", we'll match it? That seems wrong.
Attachment #621062 -
Flags: review?(21)
Assignee | ||
Comment 22•12 years ago
|
||
Yeh good call, will switch to if (e.target.rel.split(' ').indexOf('icon') !== -1) { and get the tests written, Thanks
Comment 23•12 years ago
|
||
btw, this will be bitrotted if I land bug 749018 first. But if you write the test, I'll update this code to make it work with bug 749018, once that lands.
Assignee | ||
Comment 24•12 years ago
|
||
Attachment #621062 -
Attachment is obsolete: true
Attachment #621716 -
Flags: review?(justin.lebar+bug)
Comment 25•12 years ago
|
||
+ // the rel attribute can have various space seperated values, make + // sure we only pick up correct values for 'icon' + var link = document.createElement('link'); + link.setAttribute('rel', 'shortcuticon'); + link.setAttribute('href', 'http://example.com/newicon.png'); What are you doing here? Did you mean to delete this, or otherwise use it somehow?
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #621716 -
Attachment is obsolete: true
Attachment #621716 -
Flags: review?(justin.lebar+bug)
Attachment #621767 -
Flags: review?(justin.lebar+bug)
Comment 27•12 years ago
|
||
Comment on attachment 621767 [details] Add the iconchange event (with tests) r=me, but this should be a case-insensitive search for 'icon'. (Try loading data:text/html,<link rel=ICON href="http://mozilla.org/favicon.ico"> ). Please add a test for case-insensitivity (or just modify one of your tests to check for it.)
Attachment #621767 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 28•12 years ago
|
||
Attachment #621847 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 29•12 years ago
|
||
Sorry, latest attachment was meant to obsolete old one, but added a test for case sensitivity, cheers for that catch
Comment 30•12 years ago
|
||
FYI, if you get r+ with changes, you don't need to request review on your fixed-up patch. But I guess this is your first patch, so I should be picky. :) + var relVals = e.target.rel.split(' ').map(function(x) { + return x.toLowerCase(); + }); + if (relVals.indexOf('icon') !== -1) { What about using Array.some?
Assignee | ||
Comment 31•12 years ago
|
||
Attachment #621767 -
Attachment is obsolete: true
Attachment #621847 -
Attachment is obsolete: true
Attachment #621847 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 32•12 years ago
|
||
ok sorry, and yeh Array.some makes more sense, just an old habit to avoid :)
Comment 33•12 years ago
|
||
Comment on attachment 621884 [details] [diff] [review] Add the iconchange event (with tests) I'm half-tempted to delay reviewing this so that I can get the OOP stuff landed first -- I think I just figured out the last problem. :) Anyway, this looks good. Someone is going to have to rewrite this for OOP, eventually. If I can land OOP tomorrow before dinner, I'd rather land OOP without iconchange and then add this on later. I'm happy to rewrite this myself, or you can do it if you'd prefer and have cycles. If I don't get OOP landed by dinner tomorrow, we can land this as-is. Does that sound OK to you? Also, can you push this to try, or do you need me to?
Attachment #621884 -
Flags: review+
Assignee | ||
Comment 34•12 years ago
|
||
Yeh that sounds fine, ill watch the OOP commit and rewrite when it lands, I want to get a bit of an intro into the OOP issues so this should help (its going to be pretty much the same as title changed event anyway) And yeh just reading up on tryserver so will attempt to push it there, and bug someone tomorrow if there is something I cant do
Assignee | ||
Comment 35•12 years ago
|
||
Attachment #621884 -
Attachment is obsolete: true
Comment 36•12 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=45bbcd197ae4
Comment 37•12 years ago
|
||
Try run for 45bbcd197ae4 is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=45bbcd197ae4 Results (out of 220 total builds): exception: 2 success: 197 warnings: 21 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jlebar@mozilla.com-45bbcd197ae4
Comment 38•12 years ago
|
||
If you modify the checkin message on this patch (to include "r=jlebar" and to fix the apparent "contained framesXS" business; I'm not sure what you mean here), I can check this in for you.
Assignee | ||
Comment 39•12 years ago
|
||
Attachment #622220 -
Attachment is obsolete: true
Comment 40•12 years ago
|
||
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/36c7d7fdc2ea
Assignee: nobody → dale
Status: NEW → ASSIGNED
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 41•12 years ago
|
||
I don't think we have any dev-docs on mozbrowser atm. I don't think it's a high priority; the API keeps changing...
Comment 42•12 years ago
|
||
The keyword doesn't give a priority value per se. But I removed it as the API is not stable.
Keywords: dev-doc-needed
Comment 43•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/36c7d7fdc2ea
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Reporter | ||
Comment 44•12 years ago
|
||
It turns out that just getting the favicon URL isn't really enough for all use cases. We really need to fetch and store the files in IndexedDB for history & bookmarks for example which requires CORS. We either need to change this API to return the actual favicon file as opposed to the URL (not sure how that would work for the fallback to the de-facto /favicon.ico), or rely on bug #692677 landing. Probably the latter?
Comment 45•12 years ago
|
||
> Probably the latter?
I'm thinking the latter. We have a favicon service in Firefox, but we also have a history service (Places). We're not using Places; it seems inconsistent to me to be using the favicon service.
Comment 46•12 years ago
|
||
(In fact, aiui the "favicon service" is a part of Places. :)
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 47•11 years ago
|
||
Documentation available here: https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsericonchange
Keywords: dev-doc-needed → dev-doc-complete
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•