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
Created attachment 141848 [details] Patch
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 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.
Created attachment 142943 [details] Patch
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.
Created attachment 154011 [details] Patch
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 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*
Created attachment 154273 [details] Patch
(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 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.
Created attachment 157380 [details] Patch
(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 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?
Created attachment 195703 [details] Patch
(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 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
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
Created attachment 195709 [details] Patch
Comment on attachment 195709 [details] Patch Clearing flags on attachment: 195709 Committed r147205: <http://trac.webkit.org/changeset/147205>
All reviewed patches have been landed. Closing bug.