[webkit-reviews] review denied: [Bug 24516] onchange event on form doesn't fire (or bubble up) : [Attachment 140870] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Keishi Hattori <keishi at webkit.org>'s
request for review:
Bug 24516: onchange event on form doesn't fire (or bubble up)
https://bugs.webkit.org/show_bug.cgi?id=24516

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140870&action=review


> Source/WebCore/ChangeLog:3
> +	   All the event handler attributes should be definable on HTMLElement.


Why is this different from the actual bug title?

> Source/WebCore/ChangeLog:7
> +

Please explain what caused the bug and how you're fixing it. r- due to the lack
of a change log entry.

> Source/WebCore/html/HTMLElement.cpp:250
> -    }
> -// standard events
> -    else if (attr->name() == onclickAttr) {
> +    // HTML5 event attributes
> +    // Not yet implemented: oncancel, onclose, oncuechange, onshow
> +    } else if (attr->name() == onabortAttr) {
> +	   setAttributeEventListener(eventNames().abortEvent,
createAttributeEventListener(this, attr));
> +    } else if (attr->name() == onblurAttr) {
> +	   setAttributeEventListener(eventNames().blurEvent,
createAttributeEventListener(this, attr));
> +    } else if (attr->name() == oncanplayAttr) {
> +	   setAttributeEventListener(eventNames().canplayEvent,
createAttributeEventListener(this, attr));
> +    } else if (attr->name() == oncanplaythroughAttr) {
> +	   setAttributeEventListener(eventNames().canplaythroughEvent,
createAttributeEventListener(this, attr));
> +    } else if (attr->name() == onchangeAttr) {
> +	   setAttributeEventListener(eventNames().changeEvent,
createAttributeEventListener(this, attr));
> +    } else if (attr->name() == onclickAttr) {

Why are you adding all these code? Are you sorting them? If so, that should be
done in a separate patch.
If not, then each one of these changes should be tested.