Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(370)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 22687002: Treat non-callable input as null for EventHandler attributes (Closed)

Created:
7 years, 4 months ago by do-not-use
Modified:
7 years, 4 months ago
CC:
blink-reviews, Nils Barth (inactive), kojih, jsbell+bindings_chromium.org, eae+blinkwatch, abarth-chromium, marja+watch_chromium.org, dglazkov+blink, adamk+blink_chromium.org, haraken, Nate Chapin, lgombos
Visibility:
Public.

Description

Treat non-callable input as null for EventHandler attributes Treat non-callable input as null for EventHandler attributes to match the specification: http://www.whatwg.org/specs/web-apps/current-work/multipage/webappapis.html#eventhandler http://dev.w3.org/2006/webapi/WebIDL/#TreatNonCallableAsNull This makes us in line with Firefox and closer to IE10. IE10 will not call the callback if the input is an object with an handleEvent() method. However, IE10 will not treat it as null and the value of the EventHandler attribute will be set to that object. BUG=269520 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155854

Patch Set 1 #

Patch Set 2 : Do not call v8::Null() is value is already null #

Total comments: 6

Patch Set 3 : Take Kentaro's feedback into consideration #

Total comments: 5

Patch Set 4 : Take arv's feedback into consideration #

Total comments: 2

Patch Set 5 : Fix nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+172 lines, -107 lines) Patch
M LayoutTests/fast/dom/Window/get-set-properties.html View 1 2 3 7 chunks +42 lines, -18 lines 0 comments Download
M LayoutTests/fast/dom/Window/get-set-properties-expected.txt View 2 chunks +53 lines, -50 lines 0 comments Download
D LayoutTests/fast/dom/event-attrs-isolated-world.html View 1 chunk +0 lines, -33 lines 0 comments Download
D LayoutTests/fast/dom/event-attrs-isolated-world-expected.txt View 1 chunk +0 lines, -5 lines 0 comments Download
M LayoutTests/fast/dom/exception-getting-event-handler.html View 1 chunk +1 line, -1 line 0 comments Download
A LayoutTests/fast/html/eventhandler-attribute-non-callable.html View 1 2 3 4 1 chunk +48 lines, -0 lines 0 comments Download
A LayoutTests/fast/html/eventhandler-attribute-non-callable-expected.txt View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
M Source/bindings/scripts/deprecated_code_generator_v8.pm View 1 1 chunk +3 lines, -0 lines 0 comments Download
M Source/bindings/tests/results/V8TestObject.cpp View 1 1 chunk +2 lines, -0 lines 0 comments Download
M Source/bindings/v8/V8EventListener.cpp View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
do-not-use
7 years, 4 months ago (2013-08-08 13:32:50 UTC) #1
haraken
LGTM. Needs an approval from an API reviewer. https://codereview.chromium.org/22687002/diff/4001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html File LayoutTests/fast/html/eventhandler-attribute-non-callable.html (right): https://codereview.chromium.org/22687002/diff/4001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html#newcode33 LayoutTests/fast/html/eventhandler-attribute-non-callable.html:33: o.__defineGetter__("handleEvent", ...
7 years, 4 months ago (2013-08-08 13:55:47 UTC) #2
haraken
https://codereview.chromium.org/22687002/diff/4001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html File LayoutTests/fast/html/eventhandler-attribute-non-callable.html (right): https://codereview.chromium.org/22687002/diff/4001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html#newcode5 LayoutTests/fast/html/eventhandler-attribute-non-callable.html:5: <body> Super Nit: </head> is missing.
7 years, 4 months ago (2013-08-08 13:57:34 UTC) #3
do-not-use
https://codereview.chromium.org/22687002/diff/4001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html File LayoutTests/fast/html/eventhandler-attribute-non-callable.html (right): https://codereview.chromium.org/22687002/diff/4001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html#newcode5 LayoutTests/fast/html/eventhandler-attribute-non-callable.html:5: <body> On 2013/08/08 13:57:35, haraken wrote: > > Super ...
7 years, 4 months ago (2013-08-08 14:05:47 UTC) #4
arv (Not doing code reviews)
https://codereview.chromium.org/22687002/diff/11001/LayoutTests/fast/dom/Window/get-set-properties.html File LayoutTests/fast/dom/Window/get-set-properties.html (right): https://codereview.chromium.org/22687002/diff/11001/LayoutTests/fast/dom/Window/get-set-properties.html#newcode323 LayoutTests/fast/dom/Window/get-set-properties.html:323: for (var i = 0; i < windowEventHandlers.length; i++) ...
7 years, 4 months ago (2013-08-08 14:41:27 UTC) #5
do-not-use
On 2013/08/08 14:41:27, arv wrote: > https://codereview.chromium.org/22687002/diff/11001/LayoutTests/fast/dom/Window/get-set-properties.html > File LayoutTests/fast/dom/Window/get-set-properties.html (right): > > https://codereview.chromium.org/22687002/diff/11001/LayoutTests/fast/dom/Window/get-set-properties.html#newcode323 > ...
7 years, 4 months ago (2013-08-08 17:55:29 UTC) #6
arv (Not doing code reviews)
LGTM https://codereview.chromium.org/22687002/diff/18001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html File LayoutTests/fast/html/eventhandler-attribute-non-callable.html (right): https://codereview.chromium.org/22687002/diff/18001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html#newcode33 LayoutTests/fast/html/eventhandler-attribute-non-callable.html:33: var o = { 'handleEvent': callback }; No ...
7 years, 4 months ago (2013-08-08 19:45:11 UTC) #7
do-not-use
https://codereview.chromium.org/22687002/diff/18001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html File LayoutTests/fast/html/eventhandler-attribute-non-callable.html (right): https://codereview.chromium.org/22687002/diff/18001/LayoutTests/fast/html/eventhandler-attribute-non-callable.html#newcode33 LayoutTests/fast/html/eventhandler-attribute-non-callable.html:33: var o = { 'handleEvent': callback }; On 2013/08/08 ...
7 years, 4 months ago (2013-08-08 19:53:28 UTC) #8
tkent
lgtm
7 years, 4 months ago (2013-08-08 22:55:22 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ch.dumez@sisa.samsung.com/22687002/11006
7 years, 4 months ago (2013-08-09 06:09:14 UTC) #10
commit-bot: I haz the power
7 years, 4 months ago (2013-08-09 18:28:17 UTC) #11
Message was sent while issue was closed. 
Change committed as 155854

Powered by Google App Engine
This is Rietveld 408576698