Bug 178670 – Drop confusing Event::dispatched() method

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Chris Dumez]

Drop confusing Event::dispatched() method and use Event::isBeingDispatched() instead.

[Chris Dumez]

Call sites want to check the "dispatch" flag on an event which gets set at the beginning of dispatchEvent() and unset at the end of dispatchEvent():
- https://dom.spec.whatwg.org/#dispatch-flag
- https://dom.spec.whatwg.org/#ref-for-dispatch-flag③
- https://dom.spec.whatwg.org/#dom-event-initevent

So isBeingDispatched() is the correct way of checking the Event's "dispatch" flag, dispatched() is not.

[Chris Dumez]

Created attachment 324563 [details]
Patch

[youenn fablet]

Comment on attachment 324563 [details]
Patch

I guess we can also add a test for FetchEvent at some point

[WebKit Commit Bot]

Comment on attachment 324563 [details]
Patch

Clearing flags on attachment: 324563

Committed r223849: <https://trac.webkit.org/changeset/223849>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 324563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=324563&action=review

> Source/WebCore/ChangeLog:22
> +        The right way to check the Event's "dispatch" flag is the Event::isBeingDispatched()
> +        method, so use this instead. One side effect of this change is that it is now
> +        possible to call the init*Event() method on events that have already been dispatched
> +        in order to dispatch them again, as per the specification.

I wouldn’t call this a side effect; this is the main benefit of the patch! The "dispatched" function could have been named "wasDispatched". I implemented the original rule, thinking it was correct. Maybe WebKit was always peculiar and different from other browsers. I had thought it matched them.

Some loose ends:

- This change opens the door for web content to re-use events that it did not create, using init functions to re-initialize with new values. This could may lead to problems due to properties that events have that are not normally settable by the DOM, and properties that might be hidden from it entirely. Some examples: Event::m_composed, Event::m_underlyingEvent, MouseRelatedEvent::m_isSimulated, WheelEvent::m_wheelEvent, WheelEvent::m_initializedWithPlatformWheelEvent. I think these things need to be cleared out by the init functions now; before it was safe to leave them alone because events with unusual settings for these were already dispatched, and so the init functions wouldn’t do anything to them at all. I think that any property not set by the init function may be dangerous now. Or harmless but with some strange effect. Or maybe entirely harmless.

- Looks like DOMWindow::dispatchEvent leaves the event in "isBeingDispatched" state forever. I don’t see any code to clear the target, reset the propagation flags, and set the event phase back to none. In fact, I don’t think those things should all be done with separate functions. The three dispatchEvent functions have all sorts of differences, and maybe we can refactor a bit so they share a little bit more code. No test coverage for this?

[Chris Dumez]

(In reply to Darin Adler from comment #6)
> Comment on attachment 324563 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324563&action=review
> 
> > Source/WebCore/ChangeLog:22
> > +        The right way to check the Event's "dispatch" flag is the Event::isBeingDispatched()
> > +        method, so use this instead. One side effect of this change is that it is now
> > +        possible to call the init*Event() method on events that have already been dispatched
> > +        in order to dispatch them again, as per the specification.
> 
> I wouldn’t call this a side effect; this is the main benefit of the patch!
> The "dispatched" function could have been named "wasDispatched". I
> implemented the original rule, thinking it was correct. Maybe WebKit was
> always peculiar and different from other browsers. I had thought it matched
> them.
> 
> Some loose ends:
> 
> - This change opens the door for web content to re-use events that it did
> not create, using init functions to re-initialize with new values. This
> could may lead to problems due to properties that events have that are not
> normally settable by the DOM, and properties that might be hidden from it
> entirely. Some examples: Event::m_composed, Event::m_underlyingEvent,
> MouseRelatedEvent::m_isSimulated, WheelEvent::m_wheelEvent,
> WheelEvent::m_initializedWithPlatformWheelEvent. I think these things need
> to be cleared out by the init functions now; before it was safe to leave
> them alone because events with unusual settings for these were already
> dispatched, and so the init functions wouldn’t do anything to them at all. I
> think that any property not set by the init function may be dangerous now.
> Or harmless but with some strange effect. Or maybe entirely harmless.

Per spec, initEvent() should not reset Event::m_composed:
https://dom.spec.whatwg.org/#concept-event-initialize

I'll take a look at the others.

> - Looks like DOMWindow::dispatchEvent leaves the event in
> "isBeingDispatched" state forever. I don’t see any code to clear the target,
> reset the propagation flags, and set the event phase back to none. In fact,
> I don’t think those things should all be done with separate functions. The
> three dispatchEvent functions have all sorts of differences, and maybe we
> can refactor a bit so they share a little bit more code. No test coverage
> for this?

[Radar WebKit Bug Importer]

<rdar://problem/35568528>