Bug 86363 - Add the event handler content attributes that are defined in the spec to HTMLElement
Summary: Add the event handler content attributes that are defined in the spec to HTML...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on: 113569
Blocks: 24516
  Show dependency treegraph
 
Reported: 2012-05-14 05:55 PDT by Keishi Hattori
Modified: 2013-03-29 05:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch (33.79 KB, patch)
2012-05-14 20:27 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (34.74 KB, patch)
2012-05-20 21:38 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (34.74 KB, patch)
2012-07-24 03:56 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (47.08 KB, patch)
2012-07-24 23:40 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (47.74 KB, patch)
2012-08-08 21:21 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (47.54 KB, patch)
2013-03-29 00:22 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cq-01 for chromium-linux-x86_64 (753.27 KB, application/zip)
2013-03-29 01:28 PDT, WebKit Review Bot
no flags Details
Patch (47.65 KB, patch)
2013-03-29 02:22 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-05-14 05:55:43 PDT
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
Comment 1 Keishi Hattori 2012-05-14 20:27:06 PDT
Created attachment 141848 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-14 20:30:11 PDT
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.
Comment 3 Ryosuke Niwa 2012-05-18 11:58:17 PDT
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.
Comment 4 Keishi Hattori 2012-05-20 21:38:45 PDT
Created attachment 142943 [details]
Patch
Comment 5 WebKit Review Bot 2012-05-20 21:41:11 PDT
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.
Comment 6 Keishi Hattori 2012-07-24 03:56:56 PDT
Created attachment 154011 [details]
Patch
Comment 7 Alexey Proskuryakov 2012-07-24 09:32:05 PDT
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".
Comment 8 Adam Barth 2012-07-24 10:50:07 PDT
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*
Comment 9 Keishi Hattori 2012-07-24 23:40:46 PDT
Created attachment 154273 [details]
Patch
Comment 10 Keishi Hattori 2012-07-25 13:09:05 PDT
(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.
Comment 11 Ryosuke Niwa 2012-08-07 23:01:30 PDT
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.
Comment 12 Keishi Hattori 2012-08-08 21:21:16 PDT
Created attachment 157380 [details]
Patch
Comment 13 Keishi Hattori 2012-08-08 21:21:34 PDT
(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.
Comment 14 Ryosuke Niwa 2012-09-13 15:17:02 PDT
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?
Comment 15 Keishi Hattori 2013-03-29 00:22:02 PDT
Created attachment 195703 [details]
Patch
Comment 16 Keishi Hattori 2013-03-29 00:26:59 PDT
(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.
Comment 17 WebKit Review Bot 2013-03-29 01:28:23 PDT
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
Comment 18 WebKit Review Bot 2013-03-29 01:28:26 PDT
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
Comment 19 Keishi Hattori 2013-03-29 02:22:15 PDT
Created attachment 195709 [details]
Patch
Comment 20 WebKit Review Bot 2013-03-29 02:57:27 PDT
Comment on attachment 195709 [details]
Patch

Clearing flags on attachment: 195709

Committed r147205: <http://trac.webkit.org/changeset/147205>
Comment 21 WebKit Review Bot 2013-03-29 02:57:31 PDT
All reviewed patches have been landed.  Closing bug.