Last Comment Bug 659350 - don't rely on slots to hold event handlers, IDL-ify event handlers
: don't rely on slots to hold event handlers, IDL-ify event handlers
Status: RESOLVED FIXED
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: P1 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
: 697002 (view as bug list)
Depends on: 626563 671453 675267 675405 682554 682637 684671 689564 763225
Blocks: 517143 ParisBindings 680916 414853 639720 657994 660233 676750 680903 680913 685844
  Show dependency treegraph
 
Reported: 2011-05-24 09:37 PDT by Andreas Gal :gal
Modified: 2012-08-12 10:09 PDT (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (38.16 KB, patch)
2011-05-24 09:38 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (63.62 KB, patch)
2011-05-25 04:18 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (83.38 KB, patch)
2011-05-25 08:51 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (146.44 KB, patch)
2011-05-26 11:42 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
patch (147.62 KB, patch)
2011-05-27 07:52 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
testcase (379 bytes, text/html)
2011-05-27 11:02 PDT, Olli Pettay [:smaug] (way behind * queues, especially ni? queue)
no flags Details
Part 1. Add an interface to documents to hold the touch event properties. (12.68 KB, patch)
2011-08-05 19:42 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
peterv: review+
Details | Diff | Splinter Review
part 2. Add nsITouchEventReceiver for windows. (8.52 KB, patch)
2011-08-05 19:43 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter Review
part 3. Add nsITouchEventReceiver for elements. (11.21 KB, patch)
2011-08-05 19:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter 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. (6.54 KB, patch)
2011-08-05 19:46 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter Review
part 5a. Add the new idl, with stubbed-out impls for now. (52.77 KB, patch)
2011-08-05 19:49 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
jonas: review+
Details | Diff | Splinter Review
Part 5b: main changes (96.84 KB, patch)
2011-08-05 19:59 PDT, Boris Zbarsky [:bz] (still a bit busy)
mrbkap: review+
bugs: review+
peterv: review+
Details | Diff | Splinter Review
Part 5a fixed so we pass tests (51.42 KB, patch)
2011-08-22 23:05 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Interdiff for 5a (12.80 KB, patch)
2011-08-22 23:05 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter Review
part 6. Add various tests for the on* properties. (5.32 KB, patch)
2011-08-22 23:06 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter Review

Description Andreas Gal :gal 2011-05-24 09:37:25 PDT

    
Comment 1 Andreas Gal :gal 2011-05-24 09:38:14 PDT
Created attachment 534803 [details] [diff] [review]
patch
Comment 2 :Ms2ger (⌚ UTC+1/+2) 2011-05-24 10:16:31 PDT
Would this help with bug 414853?
Comment 3 Blake Kaplan (:mrbkap) 2011-05-24 10:32:49 PDT
Yes.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-05-24 10:35:23 PDT
That seems to be passing PR_FALSE as a jsval to SetJSEventListener?
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-05-24 10:38:05 PDT
Also, how is this managing to remove BindCompiledEventHandler?
Comment 6 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-05-24 12:06:12 PDT
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.
Comment 7 Andreas Gal :gal 2011-05-25 04:18:52 PDT
Created attachment 535026 [details] [diff] [review]
patch

changes for window, body and frameset. svg still missing.
Comment 8 Andreas Gal :gal 2011-05-25 04:20:01 PDT
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 9 Andreas Gal :gal 2011-05-25 08:51:51 PDT
Created attachment 535080 [details] [diff] [review]
patch

getting closer
Comment 10 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-05-25 09:04:40 PDT
Comment on attachment 535080 [details] [diff] [review]
patch

So atm we support those onfoo listeners on all elements, not just HTML/SVG/XUL.
Comment 11 Boris Zbarsky [:bz] (still a bit busy) 2011-05-25 10:06:26 PDT
Yeah; it's not clear whether this is useful.  Does any other browser do that?
Comment 12 Andreas Gal :gal 2011-05-26 11:42:38 PDT
Created attachment 535420 [details] [diff] [review]
patch

Complete patch (XULElements might not bind the event handler properly, have to double check that). Patch compiles, but crashes horribly on startup.
Comment 13 Andreas Gal :gal 2011-05-27 07:52:19 PDT
Created attachment 535635 [details] [diff] [review]
patch
Comment 14 Andreas Gal :gal 2011-05-27 07:53:55 PDT
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).
Comment 15 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-05-27 10:09:17 PDT
Or should we just add the onfoo attributes to nsIDOMElement.idl
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2011-05-27 10:20:03 PDT
See comment 11?
Comment 17 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-05-27 11:02:47 PDT
Created attachment 535686 [details]
testcase

Seems like Opera and Chrome and M-C all support onfoo with Element.
No need for HTMLElement.
Comment 18 Andreas Gal :gal 2011-05-27 11:07:50 PDT
Should we propose to fix the spec?
Comment 19 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-05-27 11:17:32 PDT
Which spec?
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2011-05-27 11:19:11 PDT
HTML5, probably, which currently requires that these properties appear on HTMLElement.prototype.  And Web DOM Core, which should require them on Element....
Comment 21 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-05-27 11:43:07 PDT
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.idl
Webkit seems to have onfoo in the Element.idl, not in HTMLElement.idl
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2011-05-27 11:46:23 PDT
Yeah, I'm going to put this on Element.  And we just need to get the specs fixed.
Comment 23 :Ms2ger (⌚ UTC+1/+2) 2011-05-27 11:47:26 PDT
Works for me, care to email public-webapps?
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 19:42:00 PDT
Created attachment 551219 [details] [diff] [review]
Part 1.  Add an interface to documents to hold the touch event properties.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 19:43:23 PDT
Created attachment 551220 [details] [diff] [review]
part 2.  Add nsITouchEventReceiver for windows.
Comment 26 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 19:44:29 PDT
Created attachment 551221 [details] [diff] [review]
part 3.  Add nsITouchEventReceiver for elements.
Comment 27 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 19:46:36 PDT
Created 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.
Comment 28 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 19:49:40 PDT
Created attachment 551224 [details] [diff] [review]
part 5a.  Add the new idl, with stubbed-out impls for now.

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.
Comment 29 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 19:59:26 PDT
Created attachment 551226 [details] [diff] [review]
Part 5b: main changes

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.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2011-08-05 20:04:11 PDT
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.
Comment 31 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-08-21 02:20:01 PDT
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.
Comment 32 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-08-21 02:46:23 PDT
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.
Comment 33 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-08-21 02:58:42 PDT
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
Comment 34 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-08-21 04:29:19 PDT
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.
Comment 35 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 08:33:10 PDT
> 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...  :(
Comment 36 Olli Pettay [:smaug] (way behind * queues, especially ni? queue) 2011-08-22 11:01:31 PDT
(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 Peter Van der Beken [:peterv] 2011-08-22 11:19:52 PDT
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 -.
Comment 38 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 23:02:40 PDT
> 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!
Comment 39 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 23:05:03 PDT
Created attachment 555034 [details] [diff] [review]
Part 5a fixed so we pass tests
Comment 40 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 23:05:37 PDT
Created attachment 555035 [details] [diff] [review]
Interdiff for 5a
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2011-08-22 23:06:03 PDT
Created attachment 555036 [details] [diff] [review]
part 6.  Add various tests for the on* properties.
Comment 42 Blake Kaplan (:mrbkap) 2011-08-23 13:51:58 PDT
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.
Comment 43 Peter Van der Beken [:peterv] 2011-08-24 12:17:43 PDT
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(...)|.
Comment 45 Boris Zbarsky [:bz] (still a bit busy) 2011-08-24 22:03:51 PDT
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.
Comment 46 Nickolay_Ponomarev 2011-10-15 17:05:49 PDT
>> 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?
Comment 47 Boris Zbarsky [:bz] (still a bit busy) 2011-10-15 22:43:55 PDT
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.
Comment 48 Matt Brubeck (:mbrubeck) 2011-10-24 21:24:19 PDT
*** Bug 697002 has been marked as a duplicate of this bug. ***
Comment 49 Eric Shepherd [:sheppy] 2011-12-07 11:52:27 PST
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.

Note You need to log in before you can comment on or make changes to this bug.