Closed
Bug 719459
Opened 13 years ago
Closed 13 years ago
titlechange event for <iframe browser>
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla12
Tracking | Status | |
---|---|---|
firefox12 | --- | fixed |
People
(Reporter: benfrancis, Assigned: justin.lebar+bug)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [qa-])
Attachments
(2 files)
(deleted),
patch
|
smaug
:
review-
|
Details | Diff | Splinter Review |
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•13 years ago
|
Version: unspecified → Trunk
Comment 1•13 years ago
|
||
Wouldn't that cost a bit too much?
Assignee | ||
Comment 2•13 years ago
|
||
What's expensive here, you think?
Comment 3•13 years ago
|
||
(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•13 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.
Comment 5•13 years ago
|
||
(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•13 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•13 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.
Comment 8•13 years ago
|
||
Listen for inner-window-created notifications?
Assignee | ||
Comment 9•13 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?
Comment 10•13 years ago
|
||
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•13 years ago
|
||
Or add event listener to the chrome event handler?
Assignee | ||
Comment 12•13 years ago
|
||
Attachment #591207 -
Flags: review?(bugs)
Comment 13•13 years ago
|
||
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•13 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.
Comment 15•13 years ago
|
||
(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•13 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?
Comment 17•13 years ago
|
||
(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•13 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.
Comment 19•13 years ago
|
||
There is one chrome event handler per top level content browsing context.
(i.e. one chrome event handler per browser tab)
Assignee | ||
Comment 20•13 years ago
|
||
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 21•13 years ago
|
||
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•13 years ago
|
||
status-firefox12:
--- → fixed
Assignee | ||
Updated•13 years ago
|
Assignee: nobody → justin.lebar+bug
Comment 23•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 24•11 years ago
|
||
Documentation available here:
https://developer.mozilla.org/en-US/docs/Web/Reference/Events/mozbrowsertitlechange
Keywords: dev-doc-needed → dev-doc-complete
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•