Bug 32863 - [V8] NodeList should support call as function
Summary: [V8] NodeList should support call as function
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-22 02:19 PST by Johnny(Jianning) Ding
Modified: 2009-12-29 11:25 PST (History)
4 users (show)

See Also:


Attachments
patch to fix this issue (5.09 KB, patch)
2009-12-24 05:01 PST, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2 to fix this issue. (9.10 KB, patch)
2009-12-25 06:05 PST, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2 to fix this issue. (7.37 KB, patch)
2009-12-25 06:11 PST, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff
patch v2 to fix this issue (7.37 KB, patch)
2009-12-25 06:15 PST, Johnny(Jianning) Ding
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johnny(Jianning) Ding 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.
Comment 1 Johnny(Jianning) Ding 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
Comment 2 WebKit Review Bot 2009-12-24 05:05:27 PST
style-queue ran check-webkit-style on attachment 45465 [details] without any errors.
Comment 3 WebKit Review Bot 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
Comment 4 Eric Seidel (no email) 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.
Comment 5 Adam Barth 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?
Comment 6 Johnny(Jianning) Ding 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!
Comment 7 Johnny(Jianning) Ding 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.
Comment 8 Johnny(Jianning) Ding 2009-12-25 06:05:45 PST
Created attachment 45487 [details]
patch v2 to fix this issue.
Comment 9 WebKit Review Bot 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
Comment 10 Johnny(Jianning) Ding 2009-12-25 06:11:36 PST
Created attachment 45488 [details]
patch v2 to fix this issue.
Comment 11 Johnny(Jianning) Ding 2009-12-25 06:15:18 PST
Created attachment 45489 [details]
patch v2 to fix this issue
Comment 12 WebKit Review Bot 2009-12-25 06:15:59 PST
style-queue ran check-webkit-style on attachment 45489 [details] without any errors.
Comment 13 Dimitri Glazkov (Google) 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?
Comment 14 WebKit Commit Bot 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
Comment 15 Eric Seidel (no email) 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.
Comment 16 WebKit Commit Bot 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>
Comment 17 WebKit Commit Bot 2009-12-29 11:25:25 PST
All reviewed patches have been landed.  Closing bug.