Bug 70101 – Implement [Constructor] IDL for JSC

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Kentaro Hara]

Currently, all constructors of JSC are manually written as custom constructors in WebCore/bindings/js/*.cpp. We need to support [Constructor] IDL in JSC.

After this patch is landed, I am planning to replace 6 custom constructors in JSC with the [Constructor] IDL. Please see the bug 65839 for more details.

The spec of [Constructor] IDL: http://www.w3.org/TR/WebIDL/#Constructor
Implement [Constructor] IDL for V8: bug 66536

[Kentaro Hara]

Created attachment 111010 [details]
Patch

[Adam Barth]

Comment on attachment 111010 [details]
Patch

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

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3266
> +                # We do not also support a constructor [Opitional] argument without CallWithDefaultValue.

Opitional <-- typo

> Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:111
> +    ScriptExecutionContext* scriptContext = static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject())->scriptExecutionContext();
> +    if (!scriptContext)
> +        return JSValue::encode(jsUndefined());

Why is this variable check needed?  It's not used in this function.

> Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:122
> +    RefPtr<TestInterface> object = TestInterface::create(str1, str2, context, ec);

This seems to be passing context at the end instead of the beginning.

> Source/WebCore/bindings/scripts/test/JS/JSTestInterface.h:91
> +class JSTestInterfaceConstructor : public DOMConstructorObject {

I don't quite understand when this moved from the cpp file to the h file.

[Kentaro Hara]

Created attachment 111177 [details]
Patch

[Kentaro Hara]

(In reply to comment #2)
> (From update of attachment 111010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111010&action=review
> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:3266
> > +                # We do not also support a constructor [Opitional] argument without CallWithDefaultValue.
> 
> Opitional <-- typo

Fixed. 

> > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:111
> > +    ScriptExecutionContext* scriptContext = static_cast<JSDOMGlobalObject*>(exec->lexicalGlobalObject())->scriptExecutionContext();
> > +    if (!scriptContext)
> > +        return JSValue::encode(jsUndefined());
> 
> Why is this variable check needed?  It's not used in this function.

Removed this check.

> > Source/WebCore/bindings/scripts/test/JS/JSTestInterface.cpp:122
> > +    RefPtr<TestInterface> object = TestInterface::create(str1, str2, context, ec);
> 
> This seems to be passing context at the end instead of the beginning.

Sorry, fixed.

[Kentaro Hara]

Created attachment 111179 [details]
Patch

[Kentaro Hara]

Comment on attachment 111010 [details]
Patch

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

>> Source/WebCore/bindings/scripts/test/JS/JSTestInterface.h:91
>> +class JSTestInterfaceConstructor : public DOMConstructorObject {
> 
> I don't quite understand when this moved from the cpp file to the h file.

Currently, the declaration of XXXXConstructor exists on the h file if the constructor is a custom constructor, and on the cpp file if the constructor is not a custom constructor. This is inconsistent and I think that all declarations should exist on the h file.

Practical problem: For example, JSDOMWindowCustom.cpp assumes that the declaration of EventSourceConstructor exists on the h file. Currently, JSDOMWindowCustom.cpp can be compiled, since EventSourceConstructor is written as a custom constructor and thus it is declared on the h file. However, in a coming patch, I am planning to generate EventSourceConstructor by [Constructor] IDL, which will put the declaration to the cpp file (unless we move the declaration from the cpp file to the h file by this patch). This makes JSDOMWindowCustom.cpp fail to compile.

[Kentaro Hara]

Created attachment 111180 [details]
Rebased patch for review

[Adam Barth]

Comment on attachment 111180 [details]
Rebased patch for review

ok!

[WebKit Review Bot]

Comment on attachment 111180 [details]
Rebased patch for review

Clearing flags on attachment: 111180

Committed r97578: <http://trac.webkit.org/changeset/97578>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.