Last Comment Bug 719461 - iconchange event for <iframe browser>
: iconchange event for <iframe browser>
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Dale Harvey (:daleharvey)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: browser-api
  Show dependency treegraph
 
Reported: 2012-01-19 09:08 PST by Ben Francis [:benfrancis]
Modified: 2013-06-27 06:20 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add the iconchange event (1010 bytes, patch)
2012-05-04 08:46 PDT, Dale Harvey (:daleharvey)
no flags Details | Diff | Splinter Review
Add the iconchange event (with tests) (5.34 KB, patch)
2012-05-07 13:41 PDT, Dale Harvey (:daleharvey)
no flags Details | Diff | Splinter Review
Add the iconchange event (with tests) (5.39 KB, text/plain)
2012-05-07 15:55 PDT, Dale Harvey (:daleharvey)
justin.lebar+bug: review+
Details
Add the iconchange event (with tests) (5.92 KB, text/plain)
2012-05-07 20:04 PDT, Dale Harvey (:daleharvey)
no flags Details
Add the iconchange event (with tests) (5.91 KB, patch)
2012-05-07 22:22 PDT, Dale Harvey (:daleharvey)
justin.lebar+bug: review+
Details | Diff | Splinter Review
Add the iconchange event (with tests) (8.92 KB, patch)
2012-05-08 17:16 PDT, Dale Harvey (:daleharvey)
no flags Details | Diff | Splinter Review
Add the iconchange event (with tests) (8.93 KB, patch)
2012-05-09 09:25 PDT, Dale Harvey (:daleharvey)
no flags Details | Diff | Splinter Review

Description Ben Francis [:benfrancis] 2012-01-19 09:08:45 PST
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 [:fabrice] Fabrice Desré 2012-01-19 09:22:03 PST
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
Comment 2 Ben Francis [:benfrancis] 2012-01-19 09:25:56 PST
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 Justin Lebar (not reading bugmail) 2012-01-19 10:42:43 PST
> 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 Justin Lebar (not reading bugmail) 2012-01-19 10:43:19 PST
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 [:fabrice] Fabrice Desré 2012-01-19 13:53:11 PST
(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.
Comment 6 Mounir Lamouri (:mounir) 2012-01-20 02:18:38 PST
Wouldn't that cost a bit too much?
Comment 7 Justin Lebar (not reading bugmail) 2012-01-20 07:19:54 PST
(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!
Comment 8 Ben Francis [:benfrancis] 2012-01-20 07:24:34 PST
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 Mounir Lamouri (:mounir) 2012-01-20 09:09:42 PST
(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 Justin Lebar (not reading bugmail) 2012-01-20 09:12:11 PST
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 Justin Lebar (not reading bugmail) 2012-01-24 12:39:45 PST
> 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 Justin Lebar (not reading bugmail) 2012-01-24 13:07:42 PST
No other browser handles changing the <link rel='icon'>'s href either, so I guess we're in the clear.
Comment 13 Ben Francis [:benfrancis] 2012-01-25 05:09:55 PST
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 Justin Lebar (not reading bugmail) 2012-01-25 06:53:02 PST
(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.
Comment 15 Ben Francis [:benfrancis] 2012-01-26 02:45:02 PST
Sounds good to me.
Comment 16 Dale Harvey (:daleharvey) 2012-05-03 10:33:00 PDT
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
Comment 17 Dale Harvey (:daleharvey) 2012-05-04 05:52:39 PDT
Also worth remembering that a lot of sites dont have a favicon link, they just expect the browser to automatically check at root
Comment 18 Ben Francis [:benfrancis] 2012-05-04 05:55:26 PDT
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.
Comment 19 Dale Harvey (:daleharvey) 2012-05-04 08:46:12 PDT
Created attachment 621062 [details] [diff] [review]
Add the iconchange event
Comment 20 Dale Harvey (:daleharvey) 2012-05-04 08:50:54 PDT
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 Justin Lebar (not reading bugmail) 2012-05-04 08:51:49 PDT
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.
Comment 22 Dale Harvey (:daleharvey) 2012-05-04 09:15:56 PDT
Yeh good call, will switch to 

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

and get the tests written, Thanks
Comment 23 Justin Lebar (not reading bugmail) 2012-05-04 11:06:50 PDT
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.
Comment 24 Dale Harvey (:daleharvey) 2012-05-07 13:41:59 PDT
Created attachment 621716 [details] [diff] [review]
Add the iconchange event (with tests)
Comment 25 Justin Lebar (not reading bugmail) 2012-05-07 14:53:57 PDT
+      // 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?
Comment 26 Dale Harvey (:daleharvey) 2012-05-07 15:55:25 PDT
Created attachment 621767 [details]
Add the iconchange event (with tests)
Comment 27 Justin Lebar (not reading bugmail) 2012-05-07 17:54:35 PDT
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.)
Comment 28 Dale Harvey (:daleharvey) 2012-05-07 20:04:39 PDT
Created attachment 621847 [details]
Add the iconchange event (with tests)
Comment 29 Dale Harvey (:daleharvey) 2012-05-07 20:07:51 PDT
Sorry, latest attachment was meant to obsolete old one, but added a test for case sensitivity, cheers for that catch
Comment 30 Justin Lebar (not reading bugmail) 2012-05-07 22:08:40 PDT
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?
Comment 31 Dale Harvey (:daleharvey) 2012-05-07 22:22:39 PDT
Created attachment 621884 [details] [diff] [review]
Add the iconchange event (with tests)
Comment 32 Dale Harvey (:daleharvey) 2012-05-07 22:24:53 PDT
ok sorry, and yeh Array.some makes more sense, just an old habit to avoid :)
Comment 33 Justin Lebar (not reading bugmail) 2012-05-07 22:36:01 PDT
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?
Comment 34 Dale Harvey (:daleharvey) 2012-05-07 22:44:51 PDT
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
Comment 35 Dale Harvey (:daleharvey) 2012-05-08 17:16:56 PDT
Created attachment 622220 [details] [diff] [review]
Add the iconchange event (with tests)
Comment 36 Justin Lebar (not reading bugmail) 2012-05-08 17:36:51 PDT
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=45bbcd197ae4
Comment 37 Mozilla RelEng Bot 2012-05-09 05:02:18 PDT
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 Justin Lebar (not reading bugmail) 2012-05-09 09:19:28 PDT
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.
Comment 39 Dale Harvey (:daleharvey) 2012-05-09 09:25:13 PDT
Created attachment 622393 [details] [diff] [review]
Add the iconchange event (with tests)
Comment 40 Justin Lebar (not reading bugmail) 2012-05-09 09:35:32 PDT
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/36c7d7fdc2ea
Comment 41 Justin Lebar (not reading bugmail) 2012-05-09 09:45:47 PDT
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 Jean-Yves Perrier [:teoli] 2012-05-09 09:52:30 PDT
The keyword doesn't give a priority value per se. But I removed it as the API is not stable.
Comment 43 Ed Morley [:emorley] 2012-05-10 07:39:49 PDT
https://hg.mozilla.org/mozilla-central/rev/36c7d7fdc2ea
Comment 44 Ben Francis [:benfrancis] 2012-06-01 09:55:17 PDT
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 Justin Lebar (not reading bugmail) 2012-06-03 14:48:52 PDT
> 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 Justin Lebar (not reading bugmail) 2012-06-03 14:56:29 PDT
(In fact, aiui the "favicon service" is a part of Places.  :)
Comment 47 Jeremie Patonnier :Jeremie 2013-06-27 06:20:50 PDT
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsericonchange

Note You need to log in before you can comment on or make changes to this bug.