The default bug view has changed. See this FAQ.

titlechange event for <iframe browser>

RESOLVED FIXED in Firefox 12

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: benfrancis, Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
mozilla12
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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.
Version: unspecified → Trunk
Wouldn't that cost a bit too much?
(Assignee)

Comment 2

5 years ago
What's expensive here, you think?
(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.
(Assignee)

Comment 4

5 years ago
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.
(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.
(Assignee)

Comment 6

5 years ago
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.
(Assignee)

Comment 7

5 years ago
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.
Listen for inner-window-created notifications?
(Assignee)

Comment 9

5 years ago
(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?
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.
Or add event listener to the chrome event handler?
(Assignee)

Comment 12

5 years ago
Created attachment 591207 [details] [diff] [review]
Patch v1
Attachment #591207 - Flags: review?(bugs)
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)
Attachment #591207 - Flags: review?(bugs) → review-
(Assignee)

Comment 14

5 years ago
> 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.
(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.
(Assignee)

Comment 16

5 years ago
> 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?
(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.
(Assignee)

Comment 18

5 years ago
> 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.
There is one chrome event handler per top level content browsing context.
(i.e. one chrome event handler per browser tab)
(Assignee)

Comment 20

5 years ago
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.
Attachment #591934 - Flags: review?(bugs)
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.
Attachment #591934 - Flags: review?(bugs) → review+
(Assignee)

Comment 22

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f64e79dc46ab
status-firefox12: --- → fixed
(Assignee)

Updated

5 years ago
Assignee: nobody → justin.lebar+bug
https://hg.mozilla.org/mozilla-central/rev/f64e79dc46ab
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Whiteboard: [qa-]
Keywords: dev-doc-needed
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsertitlechange
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.