[webkit-reviews] review granted: [Bug 223231] [WebIDL] Fix convertRecord() to throw on enumerable symbol |key| : [Attachment 423276] 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 223231: [WebIDL] Fix convertRecord() to throw on enumerable symbol |key|
https://bugs.webkit.org/show_bug.cgi?id=223231

Attachment 423276: Patch

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




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

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

> Source/WebCore/bindings/js/JSDOMConvertStrings.cpp:38
> +    VM& vm = lexicalGlobalObject.vm();
> +    auto scope = DECLARE_THROW_SCOPE(vm);

Is there any performance benefit to moving this inside the if statement?

> Source/WebCore/bindings/js/JSDOMConvertStrings.cpp:103
> +    if (!string)
> +	   return { };

This seems like an unimportant optimization. I would leave it out; the
stringToUSVString function can handle the null string without causing any
problems. Then this can just be a one-liner. Also, we generally write
string.isNull() instead of !string. Not sure why.

    return stringToUSVString(identifierToString(lexicalGlobalObject,
identifier));