Bug 86363 – Add the event handler content attributes that are defined in the spec to HTMLElement

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Keishi Hattori]

We are missing some event handler content attributes from HTMLElement
http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#event-handlers-on-elements,-document-objects,-and-window-objects

[Keishi Hattori]

Created attachment 141848 [details]
Patch

[WebKit Review Bot]

Attachment 141848 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/html/HTMLSelectElement.cpp:315:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 141848 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141848&action=review

> Source/WebCore/ChangeLog:7
> +

Please add a description as to *why* you're making this change, and refer to the spec you're following.

[Keishi Hattori]

Created attachment 142943 [details]
Patch

[WebKit Review Bot]

Attachment 142943 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/html/HTMLSelectElement.cpp:315:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keishi Hattori]

Created attachment 154011 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 154011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154011&action=review

> Source/WebCore/html/HTMLElement.cpp:335
> +    } else if (attribute.name() == onabortAttr) {
> +        setAttributeEventListener(eventNames().abortEvent, createAttributeEventListener(this, attribute));
> +    } else if (attribute.name() == oncanplayAttr) {
> +        setAttributeEventListener(eventNames().canplayEvent, createAttributeEventListener(this, attribute));

How does this horrible long list of elseif's affect performance? From the look of it, the answer should be "negatively".

[Adam Barth]

Comment on attachment 154011 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154011&action=review

>> Source/WebCore/html/HTMLElement.cpp:335
>> +        setAttributeEventListener(eventNames().canplayEvent, createAttributeEventListener(this, attribute));
> 
> How does this horrible long list of elseif's affect performance? From the look of it, the answer should be "negatively".

This strings should be atomic already.  We might want to replace this with a HashMap keyed by StringImpl*

[Keishi Hattori]

Created attachment 154273 [details]
Patch

[Keishi Hattori]

(In reply to comment #8)
> (From update of attachment 154011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154011&action=review
> 
> >> Source/WebCore/html/HTMLElement.cpp:335
> >> +        setAttributeEventListener(eventNames().canplayEvent, createAttributeEventListener(this, attribute));
> > 
> > How does this horrible long list of elseif's affect performance? From the look of it, the answer should be "negatively".
> 
> This strings should be atomic already.  We might want to replace this with a HashMap keyed by StringImpl*

Created a HashMap, attributeNameToEventNameMap.

[Ryosuke Niwa]

Comment on attachment 154273 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=154273&action=review

> Source/WebCore/html/HTMLElement.cpp:220
> +    static HashMap<AtomicStringImpl*, AtomicString>* attributeNameToEventNameMap = 0;

We don't have to use AtomicStringImpl any more. AtomicString's hash traits will automatically take care of it for you.

> Source/WebCore/html/HTMLElement.cpp:222
> +    if (!attributeNameToEventNameMap) {
> +        attributeNameToEventNameMap = new HashMap<AtomicStringImpl*, AtomicString>;

Instead of dynamically allocating the hash map, you could have just checked the size.
Also, if we're dynamically allocating this object, then we must use OwnPtr. r- because of this.

> LayoutTests/fast/events/event-attribute.html:27
> +    var el = document.createElement(elementTags[i]);

Please don't use abbreviations like el.

[Keishi Hattori]

Created attachment 157380 [details]
Patch

[Keishi Hattori]

(In reply to comment #11)
> (From update of attachment 154273 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154273&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:220
> > +    static HashMap<AtomicStringImpl*, AtomicString>* attributeNameToEventNameMap = 0;
> 
> We don't have to use AtomicStringImpl any more. AtomicString's hash traits will automatically take care of it for you.
Done.

> > Source/WebCore/html/HTMLElement.cpp:222
> > +    if (!attributeNameToEventNameMap) {
> > +        attributeNameToEventNameMap = new HashMap<AtomicStringImpl*, AtomicString>;
> 
> Instead of dynamically allocating the hash map, you could have just checked the size.
> Also, if we're dynamically allocating this object, then we must use OwnPtr. r- because of this.
Done.

> > LayoutTests/fast/events/event-attribute.html:27
> > +    var el = document.createElement(elementTags[i]);
> 
> Please don't use abbreviations like el.
Done.

[Ryosuke Niwa]

Comment on attachment 157380 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157380&action=review

> Source/WebCore/html/HTMLElement.cpp:294
> +        attributeNameToEventNameMap.set(onwaitingAttr.localName(), eventNames().waitingEvent);
> +        attributeNameToEventNameMap.set(onwaitingAttr.localName(), eventNames().waitingEvent);

Waiting is repeated twice. Please fix it.

> LayoutTests/fast/events/event-attribute.html:16
> +var eventNames = ["onclick", "oncontextmenu", "ondblclick", "onmousedown",

Shouldn't we test all event handlers?

[Keishi Hattori]

Created attachment 195703 [details]
Patch

[Keishi Hattori]

(In reply to comment #14)
> (From update of attachment 157380 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157380&action=review
> 
> > Source/WebCore/html/HTMLElement.cpp:294
> > +        attributeNameToEventNameMap.set(onwaitingAttr.localName(), eventNames().waitingEvent);
> > +        attributeNameToEventNameMap.set(onwaitingAttr.localName(), eventNames().waitingEvent);
> 
> Waiting is repeated twice. Please fix it.
Fixed.
> > LayoutTests/fast/events/event-attribute.html:16
> > +var eventNames = ["onclick", "oncontextmenu", "ondblclick", "onmousedown",
> 
> Shouldn't we test all event handlers?
The rest don't have element.on* properties to access from JS.

[WebKit Review Bot]

Comment on attachment 195703 [details]
Patch

Rejecting attachment 195703 [details] from commit-queue.

New failing tests:
transitions/transition-end-event-unprefixed-04.html
Full output: http://webkit-commit-queue.appspot.com/results/17231844

[WebKit Review Bot]

Created attachment 195707 [details]
Archive of layout-test-results from gce-cq-01 for chromium-linux-x86_64

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: gce-cq-01  Port: chromium-linux-x86_64  Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04

[Keishi Hattori]

Created attachment 195709 [details]
Patch

[WebKit Review Bot]

Comment on attachment 195709 [details]
Patch

Clearing flags on attachment: 195709

Committed r147205: <http://trac.webkit.org/changeset/147205>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.