Bug 212767 – DOM constructor should only accept Ref<> / ExceptionOr<Ref<>> for creation to ensure toJSNewlyCreated is always returning object

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

DOM constructor should only accept Ref<> / ExceptionOr<Ref<>> for creation to ensure toJSNewlyCreated is always returning object

[Yusuke Suzuki]

Created attachment 401053 [details]
Patch

[Yusuke Suzuki]

Created attachment 401056 [details]
Patch

[Darin Adler]

Comment on attachment 401056 [details]
Patch

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

I think you need to do run-bindings-tests --reset-results and include that in your patch.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:125
>  unsigned AudioContext::s_hardwareContextCount = 0;

Do we even need to count on platforms other than Windows?

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7466
> +            if ($interface->extendedAttributes->{ConstructorMayThrowException}) {

Long term, I was thinking we could use meta-programming to generate the exception handling code only if needed, and not require an attribute just to say something may throw an exception.

If C++ is powerful enough, it would be neat if the code generator did less of the work, and there was less redundancy between the IDL and header files.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7488
> +            push(@$outputArray, "    ASSERT(jsValue);\n");
> +            push(@$outputArray, "    ASSERT(jsValue.isObject());\n");

I think the asObject function should include both of these assertions, so we should not need to repeat them in the generated code.

[Yusuke Suzuki]

Comment on attachment 401056 [details]
Patch

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

Updated the binding-test results.

>> Source/WebCore/Modules/webaudio/AudioContext.cpp:125
>>  unsigned AudioContext::s_hardwareContextCount = 0;
> 
> Do we even need to count on platforms other than Windows?

s_hardwareContextCount is also used for debugging (ASSERT in `AudioContext::uninitialize`), so I think keeping it is nice.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7466
>> +            if ($interface->extendedAttributes->{ConstructorMayThrowException}) {
> 
> Long term, I was thinking we could use meta-programming to generate the exception handling code only if needed, and not require an attribute just to say something may throw an exception.
> 
> If C++ is powerful enough, it would be neat if the code generator did less of the work, and there was less redundancy between the IDL and header files.

Agree. In long term, we should delegate these features to C++.

>> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7488
>> +            push(@$outputArray, "    ASSERT(jsValue.isObject());\n");
> 
> I think the asObject function should include both of these assertions, so we should not need to repeat them in the generated code.

Sounds good. Fixed.

[Yusuke Suzuki]

Windows failure looks unrelated since this does not change SVG.

[Yusuke Suzuki]

Committed r262583: <https://trac.webkit.org/changeset/262583>

[Radar WebKit Bug Importer]

<rdar://problem/64000845>

[Yusuke Suzuki]

I'll comment out `static_assert` in CodeGeneratorJS.pm for now since we had the same problem in Internal builds.

[Yusuke Suzuki]

Re-opened since this is blocked by bug 212799

[Yusuke Suzuki]

Created attachment 401124 [details]
Patch for landing

[EWS]

Committed r262628: <https://trac.webkit.org/changeset/262628>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401124 [details].