Bug 109678

Summary: [V8] Generate wrapper methods for custom methods
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: WebCore JavaScriptAssignee: Kentaro Hara <haraken>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, benjamin, dglazkov, japhet, ojan.autocc, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Kentaro Hara 2013-02-13 03:40:12 PST
Currently V8 directly calls back custom methods written in custom binding files. This makes it impossible for code generators to hook custom methods (e.g. Code generators cannot insert a code for FeatureObservation into custom methods). To solve the problem, we should generate wrapper methods for custom methods.

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-13 03:49:36 PST
Created attachment 188048 [details]
Patch
Comment 2 WebKit Review Bot 2013-02-13 04:51:48 PST
Comment on attachment 188048 [details]
Patch

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

New failing tests:
http/tests/xmlhttprequest/extra-parameters.html
http/tests/security/cross-frame-access-call.html
fast/dom/Window/window-early-properties-xhr.html
http/tests/appcache/update-cache.html
Comment 3 Adam Barth 2013-02-13 11:24:22 PST
Comment on attachment 188048 [details]
Patch

Looks like you've got some test failures to work through.
Comment 4 Adam Barth 2013-02-13 11:24:47 PST
Comment on attachment 188048 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188048&action=review

> Source/WebCore/page/DOMWindow.idl:183
>      [DoNotCheckSecurity, Custom] void postMessage(in SerializedScriptValue message, in DOMString targetOrigin, in Array messagePorts)

Should we mark messagePorts as optional?
Comment 5 Kentaro Hara 2013-02-13 21:37:37 PST
Created attachment 188258 [details]
Patch
Comment 6 Kentaro Hara 2013-02-13 21:38:52 PST
(In reply to comment #3)
> (From update of attachment 188048 [details])
> Looks like you've got some test failures to work through.

I think I addressed all failures.

(In reply to comment #4)
> > Source/WebCore/page/DOMWindow.idl:183
> >      [DoNotCheckSecurity, Custom] void postMessage(in SerializedScriptValue message, in DOMString targetOrigin, in Array messagePorts)
> 
> Should we mark messagePorts as optional?

Fixed. (Though technically it doesn't matter because the method is written in custom binding anyway.)
Comment 7 Adam Barth 2013-02-13 21:53:23 PST
Comment on attachment 188258 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=188258&action=review

> Source/WebCore/xml/XMLHttpRequest.idl:72
>      [Custom] void send()
>          raises(DOMException);

It's sort of too bad to lose the rest of these declarations, but I guess they're not checked by the compiler anyway.
Comment 8 Kentaro Hara 2013-02-13 22:00:37 PST
(In reply to comment #7)
> (From update of attachment 188258 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188258&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.idl:72
> >      [Custom] void send()
> >          raises(DOMException);
> 
> It's sort of too bad to lose the rest of these declarations, but I guess they're not checked by the compiler anyway.

Yeah. Ideally, we should keep the IDL declarations and custom implementation for each overloaded send() should be written in custom binding.

Currently only one custom send() implementation is written in custom binding and the send() resolves overloading manually. The fact that there are overloaded declarations in an IDL file but there is only one send() custom implementation confuses code generators.

Will fix in a follow-up patch (Will ask kojih@).
Comment 9 WebKit Review Bot 2013-02-13 22:19:08 PST
Comment on attachment 188258 [details]
Patch

Clearing flags on attachment: 188258

Committed r142849: <http://trac.webkit.org/changeset/142849>
Comment 10 WebKit Review Bot 2013-02-13 22:19:13 PST
All reviewed patches have been landed.  Closing bug.