Bug 178670 - Drop confusing Event::dispatched() method
Summary: Drop confusing Event::dispatched() method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-23 09:49 PDT by Chris Dumez
Modified: 2017-11-15 12:58 PST (History)
11 users (show)

See Also:


Attachments
Patch (19.56 KB, patch)
2017-10-23 10:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-10-23 09:49:22 PDT
Drop confusing Event::dispatched() method and use Event::isBeingDispatched() instead.
Comment 1 Chris Dumez 2017-10-23 09:52:09 PDT
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.
Comment 2 Chris Dumez 2017-10-23 10:20:22 PDT
Created attachment 324563 [details]
Patch
Comment 3 youenn fablet 2017-10-23 12:12:32 PDT
Comment on attachment 324563 [details]
Patch

I guess we can also add a test for FetchEvent at some point
Comment 4 WebKit Commit Bot 2017-10-23 12:36:28 PDT
Comment on attachment 324563 [details]
Patch

Clearing flags on attachment: 324563

Committed r223849: <https://trac.webkit.org/changeset/223849>
Comment 5 WebKit Commit Bot 2017-10-23 12:36:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 2017-10-26 10:01:21 PDT
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?
Comment 7 Chris Dumez 2017-10-26 13:27:32 PDT
(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?
Comment 8 Radar WebKit Bug Importer 2017-11-15 12:58:46 PST
<rdar://problem/35568528>