| Summary: | DOM constructor should only accept Ref<> / ExceptionOr<Ref<>> for creation to ensure toJSNewlyCreated is always returning object | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Yusuke Suzuki <ysuzuki> | ||||||||
| Component: | New Bugs | Assignee: | Yusuke Suzuki <ysuzuki> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, changseok, darin, eric.carlson, esprehn+autocc, ews-watchlist, glenn, gyuyoung.kim, jer.noble, kangil.han, keith_miller, kondapallykalyan, mark.lam, msaboff, philipj, saam, sergio, tzagallo, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Bug Depends on: | 212799 | ||||||||||
| Bug Blocks: | |||||||||||
| Attachments: |
|
||||||||||
|
Description
Yusuke Suzuki
2020-06-04 11:27:30 PDT
Created attachment 401053 [details]
Patch
Created attachment 401056 [details]
Patch
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. 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. Windows failure looks unrelated since this does not change SVG. Committed r262583: <https://trac.webkit.org/changeset/262583> I'll comment out `static_assert` in CodeGeneratorJS.pm for now since we had the same problem in Internal builds. Re-opened since this is blocked by bug 212799 Created attachment 401124 [details]
Patch for landing
Committed r262628: <https://trac.webkit.org/changeset/262628> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401124 [details]. |