Bug 109666 - [V8] Generate wrapper methods for custom getters/setters
Summary: [V8] Generate wrapper methods for custom getters/setters
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kentaro Hara
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-12 23:27 PST by Kentaro Hara
Modified: 2013-09-01 10:32 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.25 KB, patch)
2013-02-12 23:30 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff
patch for landing (9.56 KB, patch)
2013-02-13 05:57 PST, Kentaro Hara
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2013-02-12 23:27:46 PST
Currently V8 directly calls back custom getters/setters written in custom binding files. This makes it impossible for code generators to hook custom getters/setters (e.g. Code generators cannot insert a code for FeatureObservation into custom getters/setters). We should generate wrapper methods for custom getters/setters.

In the future, I will insert TRACE_EVENT() macros into these wrapper methods to profile DOM getters/setters/methods.
Comment 1 Kentaro Hara 2013-02-12 23:30:15 PST
Created attachment 188020 [details]
Patch
Comment 2 Adam Barth 2013-02-12 23:31:59 PST
Comment on attachment 188020 [details]
Patch

ok
Comment 3 Kentaro Hara 2013-02-12 23:41:01 PST
We might want to rename XXXAccessorGetter() to XXXAttrGetterCustom().
Comment 4 WebKit Review Bot 2013-02-13 00:48:12 PST
Comment on attachment 188020 [details]
Patch

Rejecting attachment 188020 [details] from commit-queue.

New failing tests:
http/tests/security/inactive-document-with-empty-security-origin.html
Full output: http://queues.webkit.org/results/16491698
Comment 5 WebKit Review Bot 2013-02-13 01:01:05 PST
Attachment 188020 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp']" exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:505:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:510:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:136:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:145:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 WebKit Review Bot 2013-02-13 01:55:39 PST
Comment on attachment 188020 [details]
Patch

Attachment 188020 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16546032

New failing tests:
http/tests/security/inactive-document-with-empty-security-origin.html
Comment 7 Kentaro Hara 2013-02-13 02:14:39 PST
Landed in r142730.
Comment 8 Kentaro Hara 2013-02-13 05:00:28 PST
Rolled out the patch in r142737 as the patch might break chromium browser tests. Will take a detailed look tomorrow.
Comment 9 Andrew Wilson 2013-02-13 05:27:20 PST
Also, this patch was resulting in timeouts running http/tests/security/inactive-document-with-empty-security-origin.html on the chromium layout test canaries.
Comment 10 Kentaro Hara 2013-02-13 05:57:20 PST
Created attachment 188064 [details]
patch for landing
Comment 11 Kentaro Hara 2013-02-13 05:58:36 PST
(In reply to comment #9)
> Also, this patch was resulting in timeouts running http/tests/security/inactive-document-with-empty-security-origin.html on the chromium layout test canaries.

Thanks. I uploaded a patch that fixes the problem. After confirming that ews bots get green, let me land it manually (to avoid style check errors).
Comment 12 WebKit Review Bot 2013-02-13 05:59:06 PST
Attachment 188064 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCore/bindings/scripts/CodeGeneratorV8.pm', u'Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp', u'Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp']" exit_code: 1
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:505:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestObj.cpp:510:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:136:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/bindings/scripts/test/V8/V8TestInterface.cpp:145:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 4 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Kentaro Hara 2013-02-13 17:38:30 PST
Landed in r142833.
Comment 14 Anders Carlsson 2013-09-01 10:32:59 PDT
V8 is gone.