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)

defect

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.
Attached patch patch (obsolete) — Splinter Review
Assignee: nobody → gal
Summary: don't rely on slots to hold event handlers → don't rely on slots to hold event handlers, IDL-ify event handlers
Would this help with bug 414853?
That seems to be passing PR_FALSE as a jsval to SetJSEventListener?
Also, how is this managing to remove BindCompiledEventHandler?
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.
Attached patch patch (obsolete) — Splinter Review
changes for window, body and frameset. svg still missing.
Attachment #534803 - Attachment is obsolete: true
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.
Attached patch patch (obsolete) — Splinter Review
getting closer
Attachment #535026 - Attachment is obsolete: true
Comment on attachment 535080 [details] [diff] [review]
patch

So atm we support those onfoo listeners on all elements, not just HTML/SVG/XUL.
Yeah; it's not clear whether this is useful.  Does any other browser do that?
Attached patch patch (obsolete) — Splinter Review
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
Attached patch patch (obsolete) — Splinter Review
Attachment #535420 - Attachment is obsolete: true
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).
Assignee: gal → bzbarsky
Or should we just add the onfoo attributes to nsIDOMElement.idl
See comment 11?
Attached file testcase
Seems like Opera and Chrome and M-C all support onfoo with Element.
No need for HTMLElement.
Attachment #535686 - Attachment is patch: false
Attachment #535686 - Attachment mime type: text/plain → text/html
Should we propose to fix the spec?
HTML5, probably, which currently requires that these properties appear on HTMLElement.prototype.  And Web DOM Core, which should require them on Element....
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.idl
Webkit seems to have onfoo in the Element.idl, not in HTMLElement.idl
Yeah, I'm going to put this on Element.  And we just need to get the specs fixed.
Works for me, care to email public-webapps?
Priority: -- → P1
Blocks: 657994
Depends on: 671453
Depends on: 675267
Depends on: 675405
Blocks: 676750
Depends on: 626563
Attachment #535635 - Attachment is obsolete: true
Attachment #551220 - Flags: review?(Olli.Pettay)
Attachment #551221 - Flags: review?(Olli.Pettay)
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)
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)
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 #551219 - Flags: review?(Olli.Pettay) → review+
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+
Attachment #551221 - Flags: review?(Olli.Pettay) → review+
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 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 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+
> 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...  :(
Blocks: 680903, 680913, 680916
Attachment #551226 - Flags: review?(peterv)
(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 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+
> 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!
Attachment #551224 - Attachment is obsolete: true
Attached patch Interdiff for 5aSplinter Review
Attachment #555035 - Flags: review?(Olli.Pettay)
Attachment #555035 - Flags: review?(Olli.Pettay) → review+
Attachment #555036 - Flags: review?(Olli.Pettay) → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
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 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+
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.
Depends on: 682554
Depends on: 682637
Depends on: 684671
Blocks: 685844
Blocks: 639720, 660233
Depends on: 689564
>> 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
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.
Whiteboard: [need review]
Blocks: 517143
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.
Depends on: 763225
Blocks: 414853
You need to log in before you can comment on or make changes to this bug.