Bug 38687 – Improve code generator scripts to pass additional ScriptExecutionContext argument to the constructor

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Jian Li]

Improve code generator scripts to support the following:
1) Pass additional ScriptExecutionContext argument to the constructor
2) Convert ScriptString

This is needed in order to avoid custom bindings code for FileReader interface.

[Jian Li]

Created attachment 55293 [details]
Proposed Patch

[Adam Barth]

Comment on attachment 55293 [details]
Proposed Patch

This is really two patches.  Can you split it in two?  Also, please add test coverage in TestObj.idl.  Other than that, these patches look good.

WebCore/bindings/scripts/CodeGeneratorJS.pm:1120
 +          push(@implContent, constructorFor($className, $protoClassName, $interfaceName, $visibleClassName, $dataNode->extendedAttributes->{"CanBeConstructed"}, $dataNode->extendedAttributes->{"CallWith"}));
I wonder if would be better to pass $dataNode here.

WebCore/bindings/v8/V8Proxy.h:412
 +              return throwError(V8Proxy::TypeError, "DOM object constructor cannot be called as a function.");
Please remove these non-localized strings.  The JSC bindings don't have them and we should aim to be identical.

WebCore/bindings/v8/V8Proxy.h:416
 +              return throwError(V8Proxy::ReferenceError, "DOM object constructor's associated frame is not available");
Please remove these non-localized strings.  The JSC bindings don't have them and we should aim to be identical.

WebCore/bindings/v8/V8Proxy.h:422
 +          obj->ref();
Manual reference twiddling is sadness.

[Jian Li]

Created attachment 55309 [details]
Proposed Patch

[Jian Li]

(In reply to comment #2)
> (From update of attachment 55293 [details])
> This is really two patches.  Can you split it in two?  Also, please add test
> coverage in TestObj.idl.  Other than that, these patches look good.

Split the patch into two. This bug is about the first change.

> 
> WebCore/bindings/scripts/CodeGeneratorJS.pm:1120
>  +          push(@implContent, constructorFor($className, $protoClassName,
> $interfaceName, $visibleClassName,
> $dataNode->extendedAttributes->{"CanBeConstructed"},
> $dataNode->extendedAttributes->{"CallWith"}));
> I wonder if would be better to pass $dataNode here.

Done.
> 
> WebCore/bindings/v8/V8Proxy.h:412
>  +              return throwError(V8Proxy::TypeError, "DOM object constructor
> cannot be called as a function.");
> Please remove these non-localized strings.  The JSC bindings don't have them
> and we should aim to be identical.

Done.
> 
> WebCore/bindings/v8/V8Proxy.h:416
>  +              return throwError(V8Proxy::ReferenceError, "DOM object
> constructor's associated frame is not available");
> Please remove these non-localized strings.  The JSC bindings don't have them
> and we should aim to be identical.

Done.

> 
> WebCore/bindings/v8/V8Proxy.h:422
>  +          obj->ref();
> Manual reference twiddling is sadness.

This is just mimicking constructDOMObject which unfortunately uses the manual refernce twiddling.

[Jian Li]

I do not change TestObj.idl to add coverage because the change is for the interface. Unless we want to add additional test interface to TestObj.idl.

[Adam Barth]

(In reply to comment #5)
> I do not change TestObj.idl to add coverage because the change is for the
> interface. Unless we want to add additional test interface to TestObj.idl.

Yeah, you can just add another IDL file next to TestObj to test interface-level features.

[Adam Barth]

Comment on attachment 55309 [details]
Proposed Patch

Thanks.  Please add a test to run-bindings-tests before landing through.

WebCore/bindings/v8/V8Proxy.h:412
 +              return throwError(V8Proxy::TypeError, "");
We should add version of throwError that doesn't need a string.  We basically never want to provide a string.

[Jian Li]

Committed as http://trac.webkit.org/changeset/58919.

(In reply to comment #7)
> (From update of attachment 55309 [details])
> Thanks.  Please add a test to run-bindings-tests before landing through.
Added.

> 
> WebCore/bindings/v8/V8Proxy.h:412
>  +              return throwError(V8Proxy::TypeError, "");
> We should add version of throwError that doesn't need a string.  We basically
> never want to provide a string.
I agree. Probably we need another patch to remove all local strings.

[Adam Barth]

> > WebCore/bindings/v8/V8Proxy.h:412
> >  +              return throwError(V8Proxy::TypeError, "");
> > We should add version of throwError that doesn't need a string.  We basically
> > never want to provide a string.
> I agree. Probably we need another patch to remove all local strings.

Turns out I think this function exists.  Just omit the string.  :)