[webkit-reviews] review granted: [Bug 211020] [WebIDL] Interface prototype objects should define @@toStringTag : [Attachment 397585] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Shvayka
<shvaikalesh at gmail.com>'s request for review:
Bug 211020: [WebIDL] Interface prototype objects should define @@toStringTag
https://bugs.webkit.org/show_bug.cgi?id=211020

Attachment 397585: Patch

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




--- Comment #6 from Darin Adler <darin at apple.com> ---
Comment on attachment 397585
  --> https://bugs.webkit.org/attachment.cgi?id=397585
Patch

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

Looks great.

Is there a more efficient way of getting this behavior without explicitly
putting the property in the prototype object at runtime? The change of the name
seems fine, but I am a little sad to see all the putDirectWithoutTransition
calls. Not really up to date enough on the latest about performance of the
JavaScript runtime, so not really sure if this is a bad thing or not.

I also have feelings about code being written out over and over again by the
code generator. I’d like to see us put a function somewhere that does this
without always repeating toStringTagSymbol and DontEnum and ReadOnly, etc. Once
we start generating code, it’s tempting to write out a lot, but I would have
wanted the generated code to be more like this:

    setUpToStringTag(vm, "CSSValueList"_s);

That function would be in a plain old C++ source file.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:4381
> +	   push(@implContent, "    putDirectWithoutTransition(vm,
vm.propertyNames->toStringTagSymbol, jsNontrivialString(vm,
\"${visibleInterfaceName}\"_s), JSC::PropertyAttribute::DontEnum |
JSC::PropertyAttribute::ReadOnly);\n");

This assumes that the visibleInterfaceName will never be a single character. If
it is a single character, then we need to call jsString rather than
jsNontrivialString. Probably a safe assumption -- what’s the chance of a single
character DOM class name? -- but maybe we should put something in the perl code
to check for this so it’s a compile time failure rather than a runtime failure.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:7321
>	   if (IsGlobalInterface($interface)) {
>	       $structureFlags{"JSC::HasStaticPropertyTable"} = 1;

Could make this a one-liner with perl's suffix-style if statement.

> LayoutTests/ChangeLog:36
> +	   * resources/idlharness.js: Remove asserts that were are commented
out in upstream.

small mistake: "were are"