719459 - titlechange event for <iframe browser>

Status: RESOLVED FIXED

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

Description

[Ben Francis [:benfrancis]]

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)]

Wouldn't that cost a bit too much?

Comment 2

[Justin Lebar (not reading bugmail)]

What's expensive here, you think?

Comment 3

[Mounir Lamouri (:mounir)]

(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)]

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)]

(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)]

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)]

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 [:bzbarsky]]

Listen for inner-window-created notifications?

Comment 9

[Justin Lebar (not reading bugmail)]

(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 [:bzbarsky]]

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][[e-mail]]]

Or add event listener to the chrome event handler?

Comment 12

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

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)]

> 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][[e-mail]]]

(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)]

> 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][[e-mail]]]

(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)]

> 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][[e-mail]]]

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)]

Attachment: 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][[e-mail]]]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f64e79dc46ab

Comment 23

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/f64e79dc46ab

Comment 24

[Jeremie Patonnier :Jeremie]

Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsertitlechange