Closed
Bug 659350
Opened 13 years ago
Closed 13 years ago
don't rely on slots to hold event handlers, IDL-ify event handlers
Categories
(Core :: DOM: Events, defect, P1)
Core
DOM: Events
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: gal, Assigned: bzbarsky)
References
(Depends on 1 open bug, Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(9 files, 6 obsolete files)
379 bytes,
text/html
|
Details | |
12.68 KB,
patch
|
smaug
:
review+
peterv
:
review+
|
Details | Diff | Splinter Review |
8.52 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
11.21 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.54 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
96.84 KB,
patch
|
mrbkap
:
review+
smaug
:
review+
peterv
:
review+
|
Details | Diff | Splinter Review |
51.42 KB,
patch
|
Details | Diff | Splinter Review | |
12.80 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
5.32 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Reporter | ||
Comment 1•13 years ago
|
||
Assignee: nobody → gal
Reporter | ||
Updated•13 years ago
|
Summary: don't rely on slots to hold event handlers → don't rely on slots to hold event handlers, IDL-ify event handlers
Comment 2•13 years ago
|
||
Would this help with bug 414853?
Comment 3•13 years ago
|
||
Yes.
Assignee | ||
Comment 4•13 years ago
|
||
That seems to be passing PR_FALSE as a jsval to SetJSEventListener?
Assignee | ||
Comment 5•13 years ago
|
||
Also, how is this managing to remove BindCompiledEventHandler?
Comment 6•13 years ago
|
||
I was thinking to do this in a bit different way, but this is probably better. nsDOMEventTargetHelper needs to be converted to use this for onfoo, and just disallow C++ onfoo listener implementations. Also, the patch is for nsIDOMHTMLElement only. SVG, XUL etc elements, documents and window need to be changed too.
Reporter | ||
Comment 7•13 years ago
|
||
changes for window, body and frameset. svg still missing.
Attachment #534803 -
Attachment is obsolete: true
Reporter | ||
Comment 8•13 years ago
|
||
BindCompiledEventHandler: the handler is now passed around as a jsval and is given to SetJSEventListener direct, it no longer looks it up on the object when the event is triggered so no need to bind there.
Comment 10•13 years ago
|
||
Comment on attachment 535080 [details] [diff] [review] patch So atm we support those onfoo listeners on all elements, not just HTML/SVG/XUL.
Assignee | ||
Comment 11•13 years ago
|
||
Yeah; it's not clear whether this is useful. Does any other browser do that?
Reporter | ||
Comment 12•13 years ago
|
||
Complete patch (XULElements might not bind the event handler properly, have to double check that). Patch compiles, but crashes horribly on startup.
Attachment #535080 -
Attachment is obsolete: true
Reporter | ||
Comment 13•13 years ago
|
||
Attachment #535420 -
Attachment is obsolete: true
Reporter | ||
Comment 14•13 years ago
|
||
XULElements register the event handler with the event manager now. Patch now crashes because of a NULL event manager. We probably have to instantiate that lazily in that spot. The other thing that is known broken is that XULElement and potentially SVG need their IDLs updated to add the event handler attributes (we only did HTML so far).
Reporter | ||
Updated•13 years ago
|
Assignee: gal → bzbarsky
Reporter | ||
Updated•13 years ago
|
Blocks: ParisBindings
Comment 15•13 years ago
|
||
Or should we just add the onfoo attributes to nsIDOMElement.idl
Assignee | ||
Comment 16•13 years ago
|
||
See comment 11?
Comment 17•13 years ago
|
||
Seems like Opera and Chrome and M-C all support onfoo with Element. No need for HTMLElement.
Updated•13 years ago
|
Attachment #535686 -
Attachment is patch: false
Attachment #535686 -
Attachment mime type: text/plain → text/html
Reporter | ||
Comment 18•13 years ago
|
||
Should we propose to fix the spec?
Comment 19•13 years ago
|
||
Which spec?
Assignee | ||
Comment 20•13 years ago
|
||
HTML5, probably, which currently requires that these properties appear on HTMLElement.prototype. And Web DOM Core, which should require them on Element....
Comment 21•13 years ago
|
||
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.idl Webkit seems to have onfoo in the Element.idl, not in HTMLElement.idl
Assignee | ||
Comment 22•13 years ago
|
||
Yeah, I'm going to put this on Element. And we just need to get the specs fixed.
Comment 23•13 years ago
|
||
Works for me, care to email public-webapps?
Assignee | ||
Updated•13 years ago
|
Priority: -- → P1
Assignee | ||
Comment 24•13 years ago
|
||
Attachment #551219 -
Flags: review?(peterv)
Attachment #551219 -
Flags: review?(Olli.Pettay)
Assignee | ||
Updated•13 years ago
|
Attachment #535635 -
Attachment is obsolete: true
Assignee | ||
Comment 25•13 years ago
|
||
Attachment #551220 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #551221 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 27•13 years ago
|
||
Attachment #551223 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 28•13 years ago
|
||
This patch can't actually land on its own, because it gives somewhat broken behavior, but I wanted to break it out from the rest of the changes for ease of review. The reason we need the separate interface for the common attributes is that otherwise some element sub-interfaces end up with too many vtable entries for xpconnect to represent in its stubs, but those interfaces are implemented in XBL so need to be stubbed.
Attachment #551224 -
Flags: review?(jonas)
Attachment #551224 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 29•13 years ago
|
||
A few other notes: 1) test_bug583225.html may no longer be testing what it wants to test; I'm not sure how to make it test that. 2) I'd appreciate a close look at my claims about cycle collection stuff in SetHandler. 3) I'd likewise appreciate a close look at the way SetJSEventListenerToJsval gets an nsIScriptContext. 4) I didn't attempt to match the spec's terminology for now; we may want to do a followup for that.
Attachment #551226 -
Flags: review?(mrbkap)
Attachment #551226 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 30•13 years ago
|
||
Oh, and my apologies for the size of that last patch. I couldn't figure out a good way to do it incrementally while keeping things compiling for each changeset without some serious gymnastics that seemed uncalled for. The good news is that so much of it is code removal... ;) Also, modulo what looks like permaorange on bug 626563, by the way, this should be passing tests. I'll figure out that permaorange before landing. There will also be a part 6 with additional tests, probably on Monday. Notes to self on that: * Test "on* in foo". * Test that onfoo properties are correctly updated when onfoo attributes are set. * Test that forwarding to the window works correctly. * Test |this| and scope handling stuff.
Whiteboard: [need review]
Attachment #551224 -
Flags: review?(jonas) → review+
Updated•13 years ago
|
Attachment #551219 -
Flags: review?(Olli.Pettay) → review+
Comment 31•13 years ago
|
||
Comment on attachment 551220 [details] [diff] [review] part 2. Add nsITouchEventReceiver for windows. >+ // XXXbz should ChromeWindow really not have touch, indexeddb, >+ // performance apis? It certainly should have touch, and performance should be ok to. Could you please add those, or file a followup to do so. Please ask bent or sicking if indexeddb is supported in chrome. > >+ // XXXbz should ModalContentWindow really not have touch, indexeddb, >+ // performance apis? It certainly should have all those. Fix or file a followup. There should be some macro for all the common window interfaces.
Attachment #551220 -
Flags: review?(Olli.Pettay) → review+
Updated•13 years ago
|
Attachment #551221 -
Flags: review?(Olli.Pettay) → review+
Comment 32•13 years ago
|
||
Comment on attachment 551223 [details] [diff] [review] part 4. Change some tests that depend on the old behavior where inline event handlers hang directly off the JSObject to not depend on it. I hope losing support for function onload() {} doesn't regress too badly.
Attachment #551223 -
Flags: review?(Olli.Pettay) → review+
Comment 33•13 years ago
|
||
Comment on attachment 551224 [details] [diff] [review] part 5a. Add the new idl, with stubbed-out impls for now. >+ [implicit_jscontext] attribute jsval onafterprint; >+ [implicit_jscontext] attribute jsval onbeforeprint; >+ [implicit_jscontext] attribute jsval onbeforeunload; >+ [implicit_jscontext] attribute jsval onhashchange; >+ [implicit_jscontext] attribute jsval onmessage; >+ [implicit_jscontext] attribute jsval onoffline; >+ [implicit_jscontext] attribute jsval ononline; >+ [implicit_jscontext] attribute jsval onpagehide; >+ [implicit_jscontext] attribute jsval onpageshow; >+ [implicit_jscontext] attribute jsval onpopstate; >+ // Not supported yet >+ // [implicit_jscontext] attribute jsval onredo; >+ [implicit_jscontext] attribute jsval onresize; >+ // Not supported yet >+ // [implicit_jscontext] attribute jsval onstorage; >+ // Not supported yet >+ // [implicit_jscontext] attribute jsval onundo; >+ [implicit_jscontext] attribute jsval onunload; Something strange with indentation. > interface nsIDOMHTMLFrameSetElement : nsIDOMHTMLElement ... >+ [implicit_jscontext] attribute jsval onpopstate; >+ // Not supported yet >+ // [implicit_jscontext] attribute jsval onredo; >+ [implicit_jscontext] attribute jsval onresize; >+ // Not supported yet >+ // [implicit_jscontext] attribute jsval onstorage; >+ // Not supported yet >+ // [implicit_jscontext] attribute jsval onundo; >+ [implicit_jscontext] attribute jsval onunload; ditto
Attachment #551224 -
Flags: review?(Olli.Pettay) → review+
Comment 34•13 years ago
|
||
Comment on attachment 551226 [details] [diff] [review] Part 5b: main changes >+#define EVENT(name_, id_, type_, struct_) \ >+ nsresult nsINode::GetOn##name_(JSContext *cx, jsval *vp) { \ >+ nsEventListenerManager *elm = GetListenerManager(PR_FALSE); \ >+ if (elm) { \ >+ elm->GetJSEventListener(nsGkAtoms::on##name_, vp); \ >+ } else { \ >+ *vp = JSVAL_NULL; \ >+ } \ >+ return NS_OK; \ >+ } \ >+ nsresult nsINode::SetOn##name_(JSContext *cx, const jsval &v) { \ >+ nsEventListenerManager *elm = GetListenerManager(PR_TRUE); \ >+ if (!elm) { \ >+ return NS_ERROR_OUT_OF_MEMORY; \ >+ } \ 'new' is infallible >+ \ >+ JSObject *obj = GetWrapper(); \ >+ if (!obj) { \ >+ return NS_ERROR_UNEXPECTED; \ >+ } \ In which case could this happen? Throwing is quite unfortunate. Could NS_ENSURE_TRUE(obj, NS_OK); be enough. > nsEventListenerManager::SetJSEventListener(nsIScriptContext *aContext, > void *aScopeObject, > nsIAtom* aName, >- PRBool aIsString, >- PRBool aPermitUntrustedEvents) >+ JSObject *aHandler, >+ PRBool aPermitUntrustedEvents, >+ nsListenerStruct **aListenerStruct) > { > nsresult rv = NS_OK; > PRUint32 eventType = nsContentUtils::GetEventId(aName); > nsListenerStruct* ls = FindJSEventListener(eventType, aName); > > if (!ls) { > // If we didn't find a script listener or no listeners existed > // create and add a new one. > nsCOMPtr<nsIDOMEventListener> scriptListener; > rv = NS_NewJSEventListener(aContext, aScopeObject, mTarget, aName, >- getter_AddRefs(scriptListener)); >+ aHandler, getter_AddRefs(scriptListener)); > if (NS_SUCCEEDED(rv)) { > AddEventListener(scriptListener, eventType, aName, nsnull, > NS_EVENT_FLAG_BUBBLE | NS_PRIV_EVENT_FLAG_SCRIPT); > > ls = FindJSEventListener(eventType, aName); > } >+ } else { >+ ls->GetJSListener()->SetHandler(aHandler); > } This is a behavior we want to change. We should remove the old listener and add a new one so that the newly added listener will be at the end of the event listener list. Please file a followup. >+ ls->mHandlerIsString = !aHandler; I hope !aHandler parameter value is documented >+ if (handlerOwner) { >+ // Always let the handler owner compile the event >+ // handler, as it may want to use a special >+ // context or scope object. >+ result = handlerOwner->CompileEventHandler(context, >+ aListenerStruct->mTypeAtom, >+ *aBody, >+ url.get(), lineNo, >+ handler); >+ } >+ else { } else { >+nsresult >+nsEventListenerManager::SetJSEventListenerToJsval(nsIAtom *aEventName, >+ JSContext *cx, >+ JSObject* aScope, >+ const jsval & v) an extra space before or after & I'd prefer nsIAtom* ... etc >+ nsIJSEventListener* GetJSListener() const { { should be in the next line >+ if (!(mFlags & NS_PRIV_EVENT_FLAG_SCRIPT)) >+ return nsnull; if (expr) { stmt; } >+ return static_cast<nsIJSEventListener *>(mListener.get()); Or, perhaps in this case expr : val1 : val2; > if (id == sOnload_id || id == sOnerror_id) { > // Make sure that this node can't go away while waiting for a > // network load that could fire an event handler. >+ // XXXbz won't this fail if the listener is added using >+ // addEventListener? On the other hand, even if I comment this >+ // code out I can't seem to reproduce the bug it was trying to >+ // fix.... > nsNodeSH::PreserveWrapper(GetNative(wrapper, obj)); > } Please file a followup to sort this out. >+ NS_IMETHODIMP nsGlobalWindow::SetOn##name_(JSContext *cx, \ >+ const jsval &v) { \ >+ nsEventListenerManager *elm = GetListenerManager(PR_TRUE); \ >+ if (!elm) { \ >+ return NS_ERROR_OUT_OF_MEMORY; \ >+ } \ >+ \ >+ JSObject *obj = mJSObject; \ >+ if (!obj) { \ >+ return NS_ERROR_UNEXPECTED; \ >+ } \ Same comments here as earlier. >@@ -113,59 +120,39 @@ NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN( > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mTarget) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_NSCOMPTR(mContext) > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS > NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END > > NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsJSEventListener) > NS_IMPL_CYCLE_COLLECTION_TRACE_MEMBER_CALLBACK(tmp->mContext->GetScriptTypeID(), > mScopeObject) >+ NS_IMPL_CYCLE_COLLECTION_TRACE_MEMBER_CALLBACK(tmp->mContext->GetScriptTypeID(), >+ mHandler) > NS_IMPL_CYCLE_COLLECTION_TRACE_END Peterv should look at this. Looking good. Changes to ELM were a bit tricky to read, I hope I didn't missed anything important.
Attachment #551226 -
Flags: review?(Olli.Pettay) → review+
Assignee | ||
Comment 35•13 years ago
|
||
> It certainly should have touch, and performance should be ok to. Filed bug 680903. > I hope losing support for function onload() {} doesn't regress too badly. It's not supported in IE or WebKit or Opera, so I hope so too. > Something strange with indentation. Will fix. > 'new' is infallible But adding an entry to a pldhash is not, and GetListenerManager can return null if that fails. So I think I do need the null-check for now. > In which case could this happen? If someone manually calls this method from C++ on an object that's never been touched from JS is the only way I can think of. I'm fine with returning NS_OK in that situation if you prefer. Let me know, please. > We should remove the old listener and add a new one > Please file a followup. Bug 680913. > I hope !aHandler parameter value is documented API comments for SetJSEventListener say: aHandler may be null to indicate that we should lazily get and compile the string for this listener. > } else { Fixed. > an extra space before or after & Removed space after. > I'd prefer nsIAtom* ... etc Hmm... I've been convinced that space before '*' is better in general, and the file is already inconsistent about it (e.g. uses nsIAtom* but for JS types puts a space before the *). If you feel strongly about it, I'll make the change, I guess. Let me know, please. > Or, perhaps in this case expr : val1 : val2; Done. > Please file a followup to sort this out. Bug 680916. > Peterv should look at this. OK. > Changes to ELM were a bit tricky to read Yeah, I'm really sorry about that... :(
Assignee | ||
Updated•13 years ago
|
Attachment #551226 -
Flags: review?(peterv)
Comment 36•13 years ago
|
||
(In reply to Boris Zbarsky (:bz) from comment #35) > But adding an entry to a pldhash is not, and GetListenerManager can return > null if that fails. So I think I do need the null-check for now. Ah, ok. > If someone manually calls this method from C++ on an object that's never > been touched from JS is the only way I can think of. I'm fine with > returning NS_OK in that situation if you prefer. Let me know, please. I'd prefer NS_OK. > Hmm... I've been convinced that space before '*' is better in general, and > the file is already inconsistent about it (e.g. uses nsIAtom* but for JS > types puts a space before the *). If you feel strongly about it, I'll make > the change, I guess. Let me know, please. No need to make the change.
Comment 37•13 years ago
|
||
Comment on attachment 551219 [details] [diff] [review] Part 1. Add an interface to documents to hold the touch event properties. Review of attachment 551219 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsDOMClassInfo.cpp @@ +2175,5 @@ > + /* Compact the interface list */ \ > + size_t count = NS_ARRAY_LENGTH(interface_list); \ > + /* count is the number of array entries, which is one greater than the */ \ > + /* number of interfaces due to the terminating null */ \ > + for (size_t i = 0; i < count-1; ++i) { \ Spaces around -.
Attachment #551219 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 38•13 years ago
|
||
> I'd prefer NS_OK. Done. > Spaces around -. Fixed. Going to post a patch with the tests. Running those found that when I moved the new properties to a tearoff all the shim gunk for body/frameset stopped working correctly. I've fixed that by just making the new nsINode methods virtual and overriding them on body and frameset as needed instead of doing the shims and will attach the updated version of part 5a and the interdiff... Yay tests!
Assignee | ||
Comment 39•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #551224 -
Attachment is obsolete: true
Assignee | ||
Comment 40•13 years ago
|
||
Attachment #555035 -
Flags: review?(Olli.Pettay)
Assignee | ||
Comment 41•13 years ago
|
||
Attachment #555036 -
Flags: review?(Olli.Pettay)
Updated•13 years ago
|
Attachment #555035 -
Flags: review?(Olli.Pettay) → review+
Updated•13 years ago
|
Attachment #555036 -
Flags: review?(Olli.Pettay) → review+
Updated•13 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Comment 42•13 years ago
|
||
Comment on attachment 551226 [details] [diff] [review] Part 5b: main changes I asked bz on IRC to add a local pointer alias for aBody in CompileEventHandlerInternal.
Attachment #551226 -
Flags: review?(mrbkap) → review+
Comment 43•13 years ago
|
||
Comment on attachment 551226 [details] [diff] [review] Part 5b: main changes Review of attachment 551226 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/events/nsJSEventListener.cpp @@ +87,5 @@ > nsContentUtils::HoldScriptObject(aContext->GetScriptTypeID(), this, > &NS_CYCLE_COLLECTION_NAME(nsJSEventListener), > aScopeObject, PR_FALSE); > + if (aHandler) { > + nsContentUtils::HoldScriptObject(aContext->GetScriptTypeID(), this, Technically this is correct, though it'll be a no-op. Once we rip out all the DOM agnostic code this should just be one call to |HoldJSObjects(this, &NS_CYCLE_COLLECTION_NAME(nsJSEventListener));| and the |DropScriptObjects(...)| calls should be |DropJSObjects(...)|.
Attachment #551226 -
Flags: review?(peterv) → review+
Comment 44•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3fbd9eaf9deb http://hg.mozilla.org/mozilla-central/rev/1f58f9ed470c http://hg.mozilla.org/mozilla-central/rev/e45c1629b10c http://hg.mozilla.org/mozilla-central/rev/2b814584bb2a http://hg.mozilla.org/mozilla-central/rev/233cdba24f03 http://hg.mozilla.org/mozilla-central/rev/a9677078eb73
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Assignee | ||
Comment 45•13 years ago
|
||
Since someone asked, the reason we're doing this is because it simplifies the code, makes us actually match the spec, fixes some longstanding web compat issues, and makes it easier to implement the new DOM bindings for elements.
Assignee | ||
Updated•13 years ago
|
Comment 46•13 years ago
|
||
>> I hope losing support for function onload() {} doesn't regress too badly.
> It's not supported in IE or WebKit or Opera, so I hope so too.
This should be mentioned in the docs. I know I was used to that construct when doing quick testing. Are there other developer-visible changes from this?
Keywords: dev-doc-needed
Assignee | ||
Comment 47•13 years ago
|
||
Er, yeah. Good catch on dev-doc-needed. There are various developer-visible changes. "in" now correctly works to test for supported on* attributes. Setting on* attributes on the relevant prototypes throws, with some exceptions that we'd rather not document because we want to remove them. The properties now claim to be accessor properties if you ask for their property descriptor (before they were either not present or data properties). You can now shadow these properties, like any other property on the prototype. Probably a bunch of others; basically all the ways IDL-defined properties differ from own data properties.
Updated•13 years ago
|
Whiteboard: [need review]
Comment 49•13 years ago
|
||
I've done some documentation work on this, as much as is likely to happen for now. The key thing is the addition of this article, which has a section that explains these changes: https://developer.mozilla.org/en/DOM/DOM_event_handlers This is also linked to from: https://developer.mozilla.org/en/DOM/document#Event_handlers https://developer.mozilla.org/en/DOM/window#Event_handlers https://developer.mozilla.org/en/DOM/element#Event_Handlers I've also added a link to that from Firefox 9 for developers. We'll add more links to this as needed as we discover places for it in the future, but for now this is documentation complete.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•