Bug 160828 – Align the event listener firing code with the latest DOM specification and simplify it

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Align the event listener firing code with the latest DOM specification:
- https://dom.spec.whatwg.org/#concept-event-listener-invoke
- https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke
- https://dom.spec.whatwg.org/#concept-event-listener

This simplifies the code a bit and makes it less error prone. There should be no web-exposed behavior change though.

[Chris Dumez]

Created attachment 285964 [details]
Patch

[Chris Dumez]

Created attachment 285967 [details]
Patch

[Chris Dumez]

Created attachment 285972 [details]
Patch

[Darin Adler]

Comment on attachment 285972 [details]
Patch

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

> Source/WebCore/dom/EventListenerMap.cpp:125
> +    m_entries.append(std::make_pair(eventType, WTFMove(listeners)));

Can we use brace syntax instead of make_pair here?

> Source/WebCore/dom/EventListenerMap.cpp:136
> +    listeners[indexOfRemovedListener]->markAsRemoved();
> +    listeners.remove(indexOfRemovedListener);

This makes me think we need a take function in Vector. Would make this code a bit more elegant. Not suggesting we do that at this time, though.

> Source/WebCore/dom/EventListenerMap.cpp:176
> +    bool foundListener = listenerVector.removeFirstMatching([] (const auto& registeredListener) {
> +        if (registeredListener->listener().wasCreatedFromMarkup()) {
> +            registeredListener->markAsRemoved();
> +            return true;
> +        }
> +        return false;
>      });

This also would read better with a takeFirstMatching function. Putting the side effect inside the predicate is tricky, not necessarily in a good way.

> Source/WebCore/dom/EventListenerMap.h:46
> +using EventListenerVector = Vector<RefPtr<RegisteredEventListener>, 1>;

Should change this header to use #pragma once.

> Source/WebCore/dom/EventTarget.cpp:208
> +class FiringEventListenersScope {
> +public:
> +    explicit FiringEventListenersScope(EventTargetData& data)
> +        : m_data(data)
> +    {
> +        m_data.isFiringEventListeners++;
> +    }
> +
> +    ~FiringEventListenersScope()
> +    {
> +        ASSERT(m_data.isFiringEventListeners);
> +        m_data.isFiringEventListeners--;
> +    }
> +
> +private:
> +    EventTargetData& m_data;
> +};

Instead of a count, this could use a boolean and a save/restore idiom. We’d then need only 1 bit, not 32 bits, in every EventTargetData. But either way seems OK.

[Chris Dumez]

Comment on attachment 285972 [details]
Patch

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

>> Source/WebCore/dom/EventTarget.cpp:208
>> +};
> 
> Instead of a count, this could use a boolean and a save/restore idiom. We’d then need only 1 bit, not 32 bits, in every EventTargetData. But either way seems OK.

It seems I could simply use the following in fireEventListeners:
TemporaryChange firingEventListenersScope(data->isFiringEventListeners, true);

Unless I am missing something.

[Chris Dumez]

Created attachment 286035 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286035 [details]
Patch

Clearing flags on attachment: 286035

Committed r204459: <http://trac.webkit.org/changeset/204459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 286035 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        1. RegisteredEventListener (equivalent to "event listener" in the spec)
> +           is now RefCounted

Might be nice to follow this up by finding a new name for EventListener and renaming RegisteredEventListener to EventListener, following our "rule of thumb" where we like to have our classes be named to match things mentioned in the specifications.

[Chris Dumez]

(In reply to comment #9)
> Comment on attachment 286035 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=286035&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        1. RegisteredEventListener (equivalent to "event listener" in the spec)
> > +           is now RefCounted
> 
> Might be nice to follow this up by finding a new name for EventListener and
> renaming RegisteredEventListener to EventListener, following our "rule of
> thumb" where we like to have our classes be named to match things mentioned
> in the specifications.

Agreed. I'll follow-up on this shortly and a couple of other suggestions you made in your earlier review.

[Chris Dumez]

(In reply to comment #10)
> (In reply to comment #9)
> > Comment on attachment 286035 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=286035&action=review
> > 
> > > Source/WebCore/ChangeLog:15
> > > +        1. RegisteredEventListener (equivalent to "event listener" in the spec)
> > > +           is now RefCounted
> > 
> > Might be nice to follow this up by finding a new name for EventListener and
> > renaming RegisteredEventListener to EventListener, following our "rule of
> > thumb" where we like to have our classes be named to match things mentioned
> > in the specifications.
> 
> Agreed. I'll follow-up on this shortly and a couple of other suggestions you
> made in your earlier review.

I am thinking:
- EventListener -> EventListenerCallback
- RegisteredEventListener -> EventListener

[Chris Dumez]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Comment on attachment 286035 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=286035&action=review
> > > 
> > > > Source/WebCore/ChangeLog:15
> > > > +        1. RegisteredEventListener (equivalent to "event listener" in the spec)
> > > > +           is now RefCounted
> > > 
> > > Might be nice to follow this up by finding a new name for EventListener and
> > > renaming RegisteredEventListener to EventListener, following our "rule of
> > > thumb" where we like to have our classes be named to match things mentioned
> > > in the specifications.
> > 
> > Agreed. I'll follow-up on this shortly and a couple of other suggestions you
> > made in your earlier review.
> 
> I am thinking:
> - EventListener -> EventListenerCallback
> - RegisteredEventListener -> EventListener

I actually don't know now. The issue is that the specification defines an "event listener" as so [1]:
"""
An event listener can be used to observe a specific event.

An event listener consists of these fields:

type (a string)
callback (an EventListener)
capture (a boolean, initially false)
passive (a boolean, initially false)
once (a boolean, initially false)
removed (a boolean for bookkeeping purposes, initially false)
"""

But then the callback field is an "EventListener" type:
https://dom.spec.whatwg.org/#callbackdef-eventlistener

Therefore, our "EventListener" naming is in line with the specification. We then use the "RegisteredEventListener" naming for what the specification refers to as "event listener" in prose.
I think it is an issue with the specification that uses basically the same naming for 2 different things.


[1] https://dom.spec.whatwg.org/#concept-event-listener

[Darin Adler]

OK, guess we leave the names alone.