Bug 96821

Summary: Support constructor-type attribute in idls other than DOMWindow.
Product: WebKit Reporter: Chang Shu <cshu>
Component: WebCore JavaScriptAssignee: Chang Shu <cshu>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, haraken, japhet, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 92413    
Attachments:
Description Flags
fix patch none

Description Chang Shu 2012-09-14 13:07:08 PDT 
Currently, it is assumed that Constructor attribute is used only in DOMWindow.idl. We should release this condition.
Bug 92413 is supposed to depend on this bug.
Comment 1 Chang Shu 2012-09-14 13:16:46 PDT 
Created attachment 164213 [details]
fix patch
Comment 2 Kentaro Hara 2012-09-14 13:30:33 PDT 
Comment on attachment 164213 [details]
fix patch

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

> Source/WebCore/ChangeLog:8
> +        In CodeGeneratorJS.pm, we should not assume only DOMWindow uses Constructor

What about CodeGeneratorV8.pm?
Comment 3 Adam Barth 2012-09-14 13:39:33 PDT 
Comment on attachment 164213 [details]
fix patch

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

>> Source/WebCore/ChangeLog:8
>> +        In CodeGeneratorJS.pm, we should not assume only DOMWindow uses Constructor
> 
> What about CodeGeneratorV8.pm?

In V8, we use info.Holder(), which probably works in either case.
Comment 4 Kentaro Hara 2012-09-14 13:45:50 PDT 
Comment on attachment 164213 [details]
fix patch

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

>>> Source/WebCore/ChangeLog:8
>>> +        In CodeGeneratorJS.pm, we should not assume only DOMWindow uses Constructor
>> 
>> What about CodeGeneratorV8.pm?
> 
> In V8, we use info.Holder(), which probably works in either case.

CodeGeneratorV8.pm uses info.Holder(), which is fine, but it looks like that CodeGeneratorV8.pm returns v8Undefined() for non-DOMWindow cases. We might want to fix the code:

http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm&exact_package=chromium&q=codegeneratorv8.pm&type=cs&l=822
Comment 5 Chang Shu 2012-09-14 14:05:10 PDT 
> > In V8, we use info.Holder(), which probably works in either case.
> 
> CodeGeneratorV8.pm uses info.Holder(), which is fine, but it looks like that CodeGeneratorV8.pm returns v8Undefined() for non-DOMWindow cases. We might want to fix the code:
> 
> http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm&exact_package=chromium&q=codegeneratorv8.pm&type=cs&l=822

The latest trunk looks like this:
http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm#L810

It's quite different from your link.
Comment 6 Kentaro Hara 2012-09-14 14:16:02 PDT 
Comment on attachment 164213 [details]
fix patch

> The latest trunk looks like this:
> http://trac.webkit.org/browser/trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm#L810

Ah, the latest trunk looks good. Sorry for the noise!
Comment 7 WebKit Review Bot 2012-09-14 14:43:17 PDT 
Comment on attachment 164213 [details]
fix patch

Clearing flags on attachment: 164213

Committed r128655: <http://trac.webkit.org/changeset/128655>
Comment 8 WebKit Review Bot 2012-09-14 14:43:21 PDT 
All reviewed patches have been landed.  Closing bug.