`EventTarget::fireEventListeners` will early return if there are no event listeners for the type of the event being dispatched. This is not the case for `DOMWindow::dispatchEvent`, which always attempts to dispatch both the capturing and bubbling phase of the event regardless of whether there are any event listeners for the dispatched event's type.
Created attachment 380485 [details] Patch
Comment on attachment 380485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380485&action=review > Source/WebCore/page/DOMWindow.cpp:2182 > + if (!hasEventListeners(event.type())) This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners(). When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK.
Comment on attachment 380485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380485&action=review >> Source/WebCore/page/DOMWindow.cpp:2182 >> + if (!hasEventListeners(event.type())) > > This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners(). > When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK. Seems like this would also be Web observable, if the JS calls dispatchEvent() on a window (which does not have a listener for |type|, then it would previously set the event's target to window. After your change, it would fail to update the Event's target, which the page's JS could observe by checking event.target after dispatching.
Comment on attachment 380485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380485&action=review >>> Source/WebCore/page/DOMWindow.cpp:2182 >>> + if (!hasEventListeners(event.type())) >> >> This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners(). >> When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK. > > Seems like this would also be Web observable, if the JS calls dispatchEvent() on a window (which does not have a listener for |type|, then it would previously set the event's target to window. After your change, it would fail to update the Event's target, which the page's JS could observe by checking event.target after dispatching. That's a good point. I didn't think about events or "legacy" events. I'll make this patch more specific to Web Inspector. Thanks!
Comment on attachment 380485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380485&action=review >>>> Source/WebCore/page/DOMWindow.cpp:2182 >>>> + if (!hasEventListeners(event.type())) >>> >>> This does not look right to me, given that firing an animationend event may cause a webkitAnimationEnd event listener to get called. See legacyType() in EventTarget.cpp, called from EventTarget::fireEventListeners(). >>> When it comes to DOM / HTML objects, I would discourage from doing such things unless the spec explicitly states it is OK. >> >> Seems like this would also be Web observable, if the JS calls dispatchEvent() on a window (which does not have a listener for |type|, then it would previously set the event's target to window. After your change, it would fail to update the Event's target, which the page's JS could observe by checking event.target after dispatching. > > That's a good point. I didn't think about events or "legacy" events. I'll make this patch more specific to Web Inspector. Thanks! s/events/custom
Created attachment 380539 [details] Patch
Can we get this reviewed and landed today?
Comment on attachment 380539 [details] Patch r=me
Comment on attachment 380539 [details] Patch LGTM
Comment on attachment 380539 [details] Patch Clearing flags on attachment: 380539 Committed r250977: <https://trac.webkit.org/changeset/250977>
All reviewed patches have been landed. Closing bug.
<rdar://problem/56161305>