[webkit-reviews] review granted: [Bug 107986] Implement pseudoElement attribute on transition DOM events. : [Attachment 184978] 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 107986: Implement pseudoElement attribute on transition DOM events.
https://bugs.webkit.org/show_bug.cgi?id=107986

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

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


The code change looks fine, some comment on the test.

> Source/WebCore/ChangeLog:18
> +

As discussed over IRC, we should also add it to the AnimationEvent interface
for consistency:

http://dev.w3.org/csswg/css3-animations/#AnimationEvent-interface

It could be done in a follow-up patch.

> Source/WebCore/ChangeLog:54
> +	   (WebCore::AnimationControllerPrivate::fireEventsAndUpdateStyle):

Please file the entries above, that would make the patch easier to read.

> LayoutTests/fast/css-generated-content/pseudo-transition-event.html:70
> +    recordedEvents.sort(compareEventInfo);
> +    expectedEvents.sort(compareEventInfo);

This is super weird for me to dispatch the events in parallel and then order
them for the purpose of comparing to the expected results. You could drive the
next transition from your event listener after having checked the event against
the expected. Both ways are equivalent, except that your approach stresses
animation in parallel vs sequential in what I proposed. All in all, this is a
NIT.

Also, let's make debugging failures easier by dispatching individual failures
instead of the generic "FAIL expectedEvents != recordedEvents".

> LayoutTests/fast/events/constructors/transition-event-constructor.html:57
> +shouldBeEqualToString("new TransitionEvent('eventType', { pseudoElement:
'::before' }).pseudoElement", "::before");
> +shouldBeEqualToString("new TransitionEvent('eventType', { pseudoElement: ''
}).pseudoElement", "");

How about testing bad values?

Like what should new TransitionEvent('eventType', { pseudoElement: 'cheese'
}).pseudoElement return?