[webkit-reviews] review denied: [Bug 115656] Regression: Event#stopPropagation() does not halt bubbling for webkitTransitionEnd : [Attachment 201742] 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 201742: Patch
https://bugs.webkit.org/attachment.cgi?id=201742&action=review

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


> Source/WebCore/dom/Event.cpp:169
> +void Event::copyEventAttributes(Event* event) const
> +{

Need a comment explaining why it’s OK to copy only the attributes in the Event
base class, and not have this be a virtual function that works properly for
classes derived from Event.

> Source/WebCore/dom/Event.h:166
> +    void copyEventAttributes(Event*) const;

It’s not obvious which direction this goes, even though there is const
involved. Name should be different.

Perhaps copyEventAttributesTo. Or copyAttributesTo. No reason to repeat the
class name. But more serious is the issue that this copies only base Event
class attributes. You’d need something different if you wanted to copy all
attributes.

> Source/WebCore/dom/EventTarget.cpp:204
> +	   RefPtr<Event> prefixedEvent = createMatchingPrefixedEvent(event);

Maybe we need new design. A single event that can be dispatched twice with
different event name rather than two events that we copy attributes between.
Seems easier to get that design right.

> LayoutTests/transitions/transition-end-event-prefixed-01.html:63
> +	   shouldBe("transitionEventContainer.type",
"transitionEventBox.type");
> +	   shouldBe("transitionEventContainer.bubbles",
"transitionEventBox.bubbles");
> +	   shouldBe("transitionEventContainer.timeStamp",
"transitionEventBox.timeStamp");
> +	   shouldBe("transitionEventContainer.cancelable",
"transitionEventBox.cancelable");
> +	   shouldBe("transitionEventContainer.srcElement",
"transitionEventBox.srcElement");
> +	   shouldBe("transitionEventContainer.returnValue",
"transitionEventBox.returnValue");
> +	   shouldBe("transitionEventContainer.cancelBubble",
"transitionEventBox.cancelBubble");
> +	   shouldBe("transitionEventContainer.defaultPrevented",
"transitionEventBox.defaultPrevented");
> +	   shouldBe("transitionEventContainer.target",
"transitionEventBox.target");
> +	   shouldBe("transitionEventContainer.currentTarget", "testContainer");

> +	   // TransitionEnd event specific properties.
> +	   shouldBe("transitionEventContainer.pseudoElement",
"transitionEventBox.pseudoElement");
> +	   shouldBe("transitionEventContainer.elapsedTime",
"transitionEventBox.elapsedTime");
> +	   shouldBe("transitionEventContainer.propertyName",
"transitionEventBox.propertyName");

The fact that these are equal doesn’t prove anything, unless we change them
all. They could just be equal by chance?

Another way of putting it is that we need 13 different FAIL when you run this
test without first patching WebKit, otherwise the test doesn’t actually test
the fix.