[webkit-reviews] review granted: [Bug 212767] DOM constructor should only accept Ref<> / ExceptionOr<Ref<>> for creation to ensure toJSNewlyCreated is always returning object : [Attachment 401056] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 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

Attachment 401056: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 401056
  --> https://bugs.webkit.org/attachment.cgi?id=401056
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.