[webkit-reviews] review granted: [Bug 105647] Unprefixed transitionend event doesn't seem to be implemented, which breaks many sites : [Attachment 182783] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has granted Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 105647: Unprefixed transitionend event doesn't seem to be implemented,
which breaks many sites
https://bugs.webkit.org/show_bug.cgi?id=105647

Attachment 182783: Patch
https://bugs.webkit.org/attachment.cgi?id=182783&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=182783&action=review


> Source/WebCore/ChangeLog:12
> +	   work on CSS Transitions. This patch adds support to save a prefixed
and
> +	   an unprefixed name in the Event class which is used later in the
event
> +	   dispatching to figure out whether we want to send the prefixed or
the
> +	   unprefixed event. In the case of the CSS Transitions, WebKit will
now

The sentence is stale: you don't store any information in the event anymore.

> Source/WebCore/ChangeLog:19
> +

Nit: Adding the thread to webkit-dev would be nice as this was discussed
broadly and no one objected or found a better alternative.

> Source/WebCore/ChangeLog:26
> +	   (WebCore::Document::addListenerTypeIfNeeded): Add support for
> +	   transitionend in case there is only an event listener on the
unprefixed
> +	   event.

I see what you meant but the wording feels weird. How about something like
(feel free to edit to your needs):

Register the prefixed listener too as transitionend listeners so that we
properly dispatch events for them.

> Source/WebCore/dom/Document.cpp:3733
> +    else if (eventType == eventNames().transitionendEvent)
> +	   addListenerType(TRANSITIONEND_LISTENER);

Nit: You could put the 2 ifs on the same line as they are logically equivalent.


if (eventType == eventNames().webkitTransitionEndEvent || eventType ==
eventNames().transitionendEvent)

> Source/WebCore/dom/EventTarget.cpp:164
> +static PassRefPtr<Event> createPrefixedEvent(const Event* event)

Nit: I would rename this to createMatchingPrefixedEvent to emphasize that it's
strictly the same event but prefixed.

> Source/WebCore/dom/EventTarget.cpp:204
> +	   prefixedEvent->setTarget(event->target());
> +	   prefixedEvent->setCurrentTarget(event->currentTarget());
> +	   prefixedEvent->setEventPhase(event->eventPhase());

This should probably go into the createPrefixedEvent as you really want to
create a cloned / match event.