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.
Created attachment 285964 [details] Patch
Created attachment 285967 [details] Patch
Created attachment 285972 [details] Patch
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.
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.
Created attachment 286035 [details] Patch
Comment on attachment 286035 [details] Patch Clearing flags on attachment: 286035 Committed r204459: <http://trac.webkit.org/changeset/204459>
All reviewed patches have been landed. Closing bug.
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.
(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.
(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
(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
OK, guess we leave the names alone.