WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(11.40 KB, patch)
2020-06-04 11:35 PDT
,
Yusuke Suzuki
darin
: review+
Details
Formatted Diff
Diff
Patch for landing
(31.87 KB, patch)
2020-06-05 00:28 PDT
,
Yusuke Suzuki
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Yusuke Suzuki
Comment 1
2020-06-04 11:33:13 PDT
Created
attachment 401053
[details]
Patch
Yusuke Suzuki
Comment 2
2020-06-04 11:35:31 PDT
Created
attachment 401056
[details]
Patch
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
Committed
r262583
: <
https://trac.webkit.org/changeset/262583
>
Radar WebKit Bug Importer
Comment 7
2020-06-04 16:11:17 PDT
<
rdar://problem/64000845
>
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.
Top of Page
Format For Printing
XML
Clone This Bug