Last Comment Bug 719459 - titlechange event for <iframe browser>
: titlechange event for <iframe browser>
Status: RESOLVED FIXED
[qa-]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on:
Blocks: browser-api
  Show dependency treegraph
 
Reported: 2012-01-19 09:05 PST by Ben Francis [:benfrancis]
Modified: 2013-06-27 06:21 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Patch v1 (8.49 KB, patch)
2012-01-24 11:59 PST, Justin Lebar (not reading bugmail)
bugs: review-
Details | Diff | Review
Patch v2 (21.94 KB, patch)
2012-01-26 13:53 PST, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Review

Description Ben Francis [:benfrancis] 2012-01-19 09:05:20 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 titlechange, when the title of the HTML document in the iframe changes.
Comment 1 Mounir Lamouri (:mounir) 2012-01-20 02:18:41 PST
Wouldn't that cost a bit too much?
Comment 2 Justin Lebar (not reading bugmail) 2012-01-20 07:40:20 PST
What's expensive here, you think?
Comment 3 Mounir Lamouri (:mounir) 2012-01-20 09:05:28 PST
(In reply to Justin Lebar [:jlebar] from comment #2)
> What's expensive here, you think?

Having to watch for title change and saving the previous title value to make sure the title has actually changed.
Comment 4 Justin Lebar (not reading bugmail) 2012-01-20 09:07:58 PST
Except in pathological cases with extremely long titles, I can't imagine how saving the previous title and comparing it to the new title would be expensive.  But if it is, we could fire titlechange when the title is set, even if the title didn't actually change.
Comment 5 Mounir Lamouri (:mounir) 2012-01-20 09:13:52 PST
(In reply to Justin Lebar [:jlebar] from comment #4)
> Except in pathological cases with extremely long titles, I can't imagine how
> saving the previous title and comparing it to the new title would be
> expensive.  But if it is, we could fire titlechange when the title is set,
> even if the title didn't actually change.

Yeah, sure. It's just that we are going to add that for *all* iframes while it's needed only for a few. In addition, watching for the title change might be a bit costy AFAICT.
Comment 6 Justin Lebar (not reading bugmail) 2012-01-20 09:19:53 PST
Like I said in bug 719461, we only need to register the observers for <iframe mozbrowser>s.  Vanilla <iframe>s don't need to watch anything.
Comment 7 Justin Lebar (not reading bugmail) 2012-01-20 13:56:18 PST
smaug/bz, what's the right way to attach a listener in the iframe down to its contentwindow?  We don't seem to know when the window changes out from under us.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-22 04:18:38 PST
Listen for inner-window-created notifications?
Comment 9 Justin Lebar (not reading bugmail) 2012-01-23 12:43:17 PST
(In reply to Boris Zbarsky (:bz) from comment #8)
> Listen for inner-window-created notifications?

You mean DOMWindowCreated?  This is fired as a chrome-only event.  I presume I can listen to from C++, but I don't see existing code which does this, and naively calling window->AddEventListener doesn't seem to work.  Is there a trick?
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-01-24 00:13:13 PST
Event listeners get cleared on a load, so addEventListener won't work.  But from C++ you can listen to the "content-document-global-created" notification on nsIObserverService.  I think we have some devmo docs for it, even.
Comment 11 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-24 00:37:22 PST
Or add event listener to the chrome event handler?
Comment 12 Justin Lebar (not reading bugmail) 2012-01-24 11:59:30 PST
Created attachment 591207 [details] [diff] [review]
Patch v1
Comment 13 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-24 14:41:15 PST
Comment on attachment 591207 [details] [diff] [review]
Patch v1

I don't see any place where you remove the event listener.
Note, chromehandler can live a lot longer than the html:iframe element.

Also, you want to use capture phase so that content can't just
call stopPropagation to make this all not work.
(Though, I don't recall if DOMTitleChanged is actually dispatched to content,
or only to chromehandler)
Comment 14 Justin Lebar (not reading bugmail) 2012-01-24 14:45:50 PST
> I don't see any place where you remove the event listener.
> Note, chromehandler can live a lot longer than the html:iframe element.

Do I need to remove the listener, even if the iframe element dies?  Is there harm in keeping it around?

> (Though, I don't recall if DOMTitleChanged is actually dispatched to content,
> or only to chromehandler)

It's chrome-only afaict, but I'll change it.
Comment 15 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-24 14:47:57 PST
(In reply to Justin Lebar [:jlebar] from comment #14)
> > I don't see any place where you remove the event listener.
> > Note, chromehandler can live a lot longer than the html:iframe element.
> 
> Do I need to remove the listener, even if the iframe element dies?  Is there
> harm in keeping it around?

Yes, it is harmful to keep effectively non-operational objects alive to consume memory and slowing
down event handling.
If a new <html:iframe mozbrowser> is added, it will add another listener.
Comment 16 Justin Lebar (not reading bugmail) 2012-01-24 14:57:49 PST
> Yes, it is harmful to keep effectively non-operational objects alive to consume memory and slowing
> down event handling.

The amount of memory here (one weak ref plus a vtable) is trivial compared to the memory of just an nsGenericHTMLFrameElement object, not to mention all the stuff that comes with an iframe.

How many events might this hear after the element dies?

If we're really concerned about reducing overhead, we'd do a lot better not to add this listener for non-mozbrowser iframes.  I didn't do that because I didn't think it matters.  It's never more than one listener per iframe, right?

> If a new <html:iframe mozbrowser> is added, it will add another listener.

Sure, on a different iframe.  That's not redundant, right?
Comment 17 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-24 15:03:32 PST
(In reply to Justin Lebar [:jlebar] from comment #16)
> How many events might this hear after the element dies?
You aren't adding the listener to the iframe element, but to chrome event handler.
It is an object in chrome, and stays alive until xul:browser is removed from chrome document.


> If we're really concerned about reducing overhead, we'd do a lot better not
> to add this listener for non-mozbrowser iframes.
Oh, right, you must not add it to non-mozbrowser iframes.
Comment 18 Justin Lebar (not reading bugmail) 2012-01-24 15:13:25 PST
> You aren't adding the listener to the iframe element, but to chrome event handler.

Oh, I see.  There's one global chrome event handler!

Sounds like maybe I should have just one listener and figure out which frame element to notify.
Comment 19 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-24 15:36:39 PST
There is one chrome event handler per top level content browsing context.
(i.e. one chrome event handler per browser tab)
Comment 20 Justin Lebar (not reading bugmail) 2012-01-26 13:53:24 PST
Created attachment 591934 [details] [diff] [review]
Patch v2

Lazily attaching the listeners necessitated changes to all the existing tests: You have to create the iframe (or at least set mozbrowser=true) after the prefs are set to allow browser frames.
Comment 21 Olli Pettay [:smaug] (high review load, please consider other reviewers) 2012-01-26 14:35:20 PST
Comment on attachment 591934 [details] [diff] [review]
Patch v2

don't use mozilla::content::internal but move the class inside
nsGenericHTMLFrameElement as discussed on IRC.
Comment 22 Justin Lebar (not reading bugmail) 2012-01-26 16:32:19 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/f64e79dc46ab
Comment 23 Matt Brubeck (:mbrubeck) 2012-01-27 09:11:47 PST
https://hg.mozilla.org/mozilla-central/rev/f64e79dc46ab
Comment 24 Jeremie Patonnier :Jeremie 2013-06-27 06:21:20 PDT
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsertitlechange

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