[webkit-reviews] review denied: [Bug 115656] Regression: Event#stopPropagation() does not halt bubbling for webkitTransitionEnd : [Attachment 201728] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 115656: Regression: Event#stopPropagation() does not halt bubbling for
webkitTransitionEnd
https://bugs.webkit.org/show_bug.cgi?id=115656

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=201728&action=review


Looks good, needs more work on test coverage.

> Source/WebCore/ChangeLog:15
> +	   Test: transitions/transition-end-event-prefixed-01.html

This test has insufficient coverage. We need to test coverage of all 9 of the
things that are copied from the prefixed event to the original event. If I
delete any one of the 9 from the “sync” function then the test should fail. If
you can’t cover all 9 you should at least get as close as you can. The test
will probably demonstrate that we don’t need to copy all 9, which is a good
thing for us to know!

> Source/WebCore/ChangeLog:19
> +	   (WebCore):

This bogus line should be removed.

> Source/WebCore/dom/EventTarget.cpp:165
> +static void syncEventAttributes(const Event* sourceEvent, Event*
targetEvent)

The word “sync” sounds like a bidirectional operation with some kind of
merging. I suggest a different name for this function that sounds directional.
Maybe the word “propagate” or “copy” or “update”.

I’d maybe call this copyEventAttributesForPairedEvents, or something like that.


I don’t think sourceEvent and targetEvent are good names for the arguments
because the word “target” has a specific name in the context of events.

It’s also non-obvious that this copies enough of the attributes. It seems this
set is good enough for the transition end event, but is it good enough for all
future events we might want to use this on?

> Source/WebCore/dom/EventTarget.cpp:211
> +    RefPtr<Event> prefixedEvent;

Why does this need to be defined outside the if statement? Is there some reason
we need to keep it alive after the block it’s allocated and used in?