Bug 32863

Summary: [V8] NodeList should support call as function
Product: WebKit Reporter: Johnny(Jianning) Ding <jnd>
Component: DOMAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, dglazkov, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch to fix this issue
none
patch v2 to fix this issue.
none
patch v2 to fix this issue.
none
patch v2 to fix this issue none

Johnny(Jianning) Ding
Reported 2009-12-22 02:19:15 PST
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 2009-12-24 05:01:01 PST
Created attachment 45465 [details] patch to fix this issue Implemented the V8NodeList::callAsFunctionCallback by referring callNodeList in JSNodeListCustom.cpp
WebKit Review Bot
Comment 2 2009-12-24 05:05:27 PST
style-queue ran check-webkit-style on attachment 45465 [details] without any errors.
WebKit Review Bot
Comment 3 2009-12-24 05:19:24 PST
Attachment 45465 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/142726
Eric Seidel (no email)
Comment 4 2009-12-24 11:53:16 PST
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 2009-12-24 14:18:17 PST
(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 2009-12-25 06:03:56 PST
(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 2009-12-25 06:04:58 PST
(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 2009-12-25 06:05:45 PST
Created attachment 45487 [details] patch v2 to fix this issue.
WebKit Review Bot
Comment 9 2009-12-25 06:10:36 PST
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 2009-12-25 06:11:36 PST
Created attachment 45488 [details] patch v2 to fix this issue.
Johnny(Jianning) Ding
Comment 11 2009-12-25 06:15:18 PST
Created attachment 45489 [details] patch v2 to fix this issue
WebKit Review Bot
Comment 12 2009-12-25 06:15:59 PST
style-queue ran check-webkit-style on attachment 45489 [details] without any errors.
Dimitri Glazkov (Google)
Comment 13 2009-12-29 08:57:47 PST
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 2009-12-29 11:00:53 PST
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 2009-12-29 11:17:03 PST
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 2009-12-29 11:25:20 PST
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 2009-12-29 11:25:25 PST
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.