Closed Bug 719461 Opened 12 years ago Closed 12 years ago

iconchange event for <iframe browser>

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: benfrancis, Assigned: daleharvey)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 6 obsolete files)

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.
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
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.
> 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?
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.
(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.
Version: unspecified → Trunk
Wouldn't that cost a bit too much?
(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!
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?
(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.
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?
> 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.
No other browser handles changing the <link rel='icon'>'s href either, so I guess we're in the clear.
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?
(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.
Sounds good to me.
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
Also worth remembering that a lot of sites dont have a favicon link, they just expect the browser to automatically check at root
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.
Attached patch Add the iconchange event (obsolete) — Splinter Review
Attachment #621062 - Flags: review?(21)
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 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)
Yeh good call, will switch to 

  if (e.target.rel.split(' ').indexOf('icon') !== -1) {

and get the tests written, Thanks
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.
Attachment #621062 - Attachment is obsolete: true
Attachment #621716 - Flags: review?(justin.lebar+bug)
+      // 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?
Attached file Add the iconchange event (with tests) (obsolete) —
Attachment #621716 - Attachment is obsolete: true
Attachment #621716 - Flags: review?(justin.lebar+bug)
Attachment #621767 - Flags: review?(justin.lebar+bug)
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+
Attached file Add the iconchange event (with tests) (obsolete) —
Attachment #621847 - Flags: review?(justin.lebar+bug)
Sorry, latest attachment was meant to obsolete old one, but added a test for case sensitivity, cheers for that catch
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?
Attachment #621767 - Attachment is obsolete: true
Attachment #621847 - Attachment is obsolete: true
Attachment #621847 - Flags: review?(justin.lebar+bug)
ok sorry, and yeh Array.some makes more sense, just an old habit to avoid :)
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+
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
Attachment #621884 - Attachment is obsolete: true
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
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.
Attachment #622220 - Attachment is obsolete: true
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/36c7d7fdc2ea
Assignee: nobody → dale
Status: NEW → ASSIGNED
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...
The keyword doesn't give a priority value per se. But I removed it as the API is not stable.
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/36c7d7fdc2ea
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
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?
> 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.
(In fact, aiui the "favicon service" is a part of Places.  :)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: