RESOLVED FIXED 32863
[V8] NodeList should support call as function 
https://bugs.webkit.org/show_bug.cgi?id=32863
Summary [V8] NodeList should support call as function
Johnny(Jianning) Ding
Reported Tuesday, December 22, 2009 10:19:15 AM UTC
After running the following html snippet in both Safari and Chrome, Safari passed but Chrome didn't 01.<!DOCTYPE HTML> 02.<HTML> 03.<BODY> 04.<SPAN>contents 1</SPAN> <SPAN>contents 2</SPAN> 05.<SCRIPT> 06.var list = document.getElementsByTagName("span"); 07.alert(list[0]); 08.alert(list(0)); 09.</SCRIPT> 10.</BODY> 11.</HTML> The root cause of this issue is because in V8 binding, the NodeList object didn't support call as function, but Safari's JS binding supports. See the following js code. In Safari NodeList is strange object to support both get and call, NodeList[0] and NodeList(0) both work. But now Chrome will throw an exception when running at line 8. We should change the V8 binding code to match Safari. I will provide a patch and corresponding test.
Attachments
patch to fix this issue (5.09 KB, patch)
2009-12-24 05:01 PST, Johnny(Jianning) Ding 		no flags
DetailsFormatted DiffDiff
patch v2 to fix this issue. (9.10 KB, patch)
2009-12-25 06:05 PST, Johnny(Jianning) Ding 		no flags
DetailsFormatted DiffDiff
patch v2 to fix this issue. (7.37 KB, patch)
2009-12-25 06:11 PST, Johnny(Jianning) Ding 		no flags
DetailsFormatted DiffDiff
patch v2 to fix this issue (7.37 KB, patch)
2009-12-25 06:15 PST, Johnny(Jianning) Ding 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Johnny(Jianning) Ding
Comment 1 Thursday, December 24, 2009 1:01:01 PM UTC
Created attachment 45465 [details] patch to fix this issue Implemented the V8NodeList::callAsFunctionCallback by referring callNodeList in JSNodeListCustom.cpp
WebKit Review Bot
Comment 2 Thursday, December 24, 2009 1:05:27 PM UTC
style-queue ran check-webkit-style on attachment 45465 [details] without any errors.
WebKit Review Bot
Comment 3 Thursday, December 24, 2009 1:19:24 PM UTC
Attachment 45465 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/142726
Eric Seidel (no email)
Comment 4 Thursday, December 24, 2009 7:53:16 PM UTC
Comment on attachment 45465 [details] patch to fix this issue Seems this should be a real script-test (i.e. the html should just be autogenerated): http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree dglazkov is probably your best reviwer here.
Adam Barth
Comment 5 Thursday, December 24, 2009 10:18:17 PM UTC
(In reply to comment #3) > Attachment 45465 [details] did not build on chromium: > Build output: http://webkit-commit-queue.appspot.com/results/142726 Any thoughts on this build failure? Is this a mistake in the patch or a mistake by the bot?
Johnny(Jianning) Ding
Comment 6 Friday, December 25, 2009 2:03:56 PM UTC
(In reply to comment #4) > (From update of attachment 45465 [details]) > Seems this should be a real script-test (i.e. the html should just be > autogenerated): > http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree > > dglazkov is probably your best reviwer here. Done, Thx and Merry XMas!
Johnny(Jianning) Ding
Comment 7 Friday, December 25, 2009 2:04:58 PM UTC
(In reply to comment #5) > (In reply to comment #3) > > Attachment 45465 [details] [details] did not build on chromium: > > Build output: http://webkit-commit-queue.appspot.com/results/142726 > > Any thoughts on this build failure? Is this a mistake in the patch or a > mistake by the bot? My mistake, miss the .pm file. A new patch will be uploaded soon.
Johnny(Jianning) Ding
Comment 8 Friday, December 25, 2009 2:05:45 PM UTC
Created attachment 45487 [details] patch v2 to fix this issue.
WebKit Review Bot
Comment 9 Friday, December 25, 2009 2:10:36 PM UTC
Attachment 45487 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebCore/bindings/v8/V8DOMWrapper.cpp:352: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3] Total errors found: 1
Johnny(Jianning) Ding
Comment 10 Friday, December 25, 2009 2:11:36 PM UTC
Created attachment 45488 [details] patch v2 to fix this issue.
Johnny(Jianning) Ding
Comment 11 Friday, December 25, 2009 2:15:18 PM UTC
Created attachment 45489 [details] patch v2 to fix this issue
WebKit Review Bot
Comment 12 Friday, December 25, 2009 2:15:59 PM UTC
style-queue ran check-webkit-style on attachment 45489 [details] without any errors.
Dimitri Glazkov (Google)
Comment 13 Tuesday, December 29, 2009 4:57:47 PM UTC
Comment on attachment 45489 [details] patch v2 to fix this issue great, thanks! Are you sure there wasn't already a test for this?
WebKit Commit Bot
Comment 14 Tuesday, December 29, 2009 7:00:53 PM UTC
Comment on attachment 45489 [details] patch v2 to fix this issue Rejecting patch 45489 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--quiet']" exit_code: 1 Running build-dumprendertree Running tests from /Users/eseidel/Projects/CommitQueueSVN/LayoutTests Testing 11854 test cases. fast/css/non-standard-checkbox-size.html -> failed Exiting early after 1 failures. 5218 tests run. 79.16s total testing time 5217 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 1 test case (<1%) had stderr output Full output: http://webkit-commit-queue.appspot.com/results/150773
Eric Seidel (no email)
Comment 15 Tuesday, December 29, 2009 7:17:03 PM UTC
Comment on attachment 45489 [details] patch v2 to fix this issue Sorry. We were bit by bug 28603. I've fixed the commit bot for now.
WebKit Commit Bot
Comment 16 Tuesday, December 29, 2009 7:25:20 PM UTC
Comment on attachment 45489 [details] patch v2 to fix this issue Clearing flags on attachment: 45489 Committed r52627: <http://trac.webkit.org/changeset/52627>
WebKit Commit Bot
Comment 17 Tuesday, December 29, 2009 7:25:25 PM UTC
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.