RESOLVED FIXED 212767
DOM constructor should only accept Ref<> / ExceptionOr<Ref<>> for creation to ensure toJSNewlyCreated is always returning object
https://bugs.webkit.org/show_bug.cgi?id=212767
Summary DOM constructor should only accept Ref<> / ExceptionOr<Ref<>> for creation to...
Yusuke Suzuki
Reported 2020-06-04 11:27:30 PDT
DOM constructor should only accept Ref<> / ExceptionOr<Ref<>> for creation to ensure toJSNewlyCreated is always returning object
Attachments
Patch (11.53 KB, patch)
2020-06-04 11:33 PDT, Yusuke Suzuki
no flags
Patch (11.40 KB, patch)
2020-06-04 11:35 PDT, Yusuke Suzuki
darin: review+
Patch for landing (31.87 KB, patch)
2020-06-05 00:28 PDT, Yusuke Suzuki
no flags
Yusuke Suzuki
Comment 1 2020-06-04 11:33:13 PDT
Yusuke Suzuki
Comment 2 2020-06-04 11:35:31 PDT
Darin Adler
Comment 3 2020-06-04 12:00:23 PDT
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 4 2020-06-04 15:46:23 PDT
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
Comment 5 2020-06-04 16:07:40 PDT
Windows failure looks unrelated since this does not change SVG.
Yusuke Suzuki
Comment 6 2020-06-04 16:11:02 PDT
Radar WebKit Bug Importer
Comment 7 2020-06-04 16:11:17 PDT
Yusuke Suzuki
Comment 8 2020-06-04 21:33:14 PDT
I'll comment out `static_assert` in CodeGeneratorJS.pm for now since we had the same problem in Internal builds.
Yusuke Suzuki
Comment 9 2020-06-04 21:41:15 PDT
Re-opened since this is blocked by bug 212799
Yusuke Suzuki
Comment 10 2020-06-05 00:28:23 PDT
Created attachment 401124 [details] Patch for landing
EWS
Comment 11 2020-06-05 10:26:19 PDT
Committed r262628: <https://trac.webkit.org/changeset/262628> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401124 [details].
Note You need to log in before you can comment on or make changes to this bug.