[webkit-reviews] review granted: [Bug 215047] JSClassRef should work with JS class syntax. : [Attachment 407223] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 25 14:37:45 PDT 2020


Darin Adler <darin at apple.com> has granted Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 215047: JSClassRef should work with JS class syntax.
https://bugs.webkit.org/show_bug.cgi?id=215047

Attachment 407223: Patch

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




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

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

> Source/JavaScriptCore/ChangeLog:15
> +	   the new.target. Ideally we would have passed the derived classes
> +	   constructor from the beginning of our support for JS subclassing
> +	   but at this point that's probably not compatible with too many
> +	   applications.

We can create an old compatible version and a new version designed the way we
like. We don’t have to give up on it forever.

> Source/JavaScriptCore/API/APICallbackFunction.h:91
> +	   // If we are doing a derived class construction get the .prototype
property off the new target first so we look behave closer to normal JS.

"look behave" -> "behave"?

> Source/JavaScriptCore/API/APICallbackFunction.h:119
> +	   // This won't trigger proxy traps on newObject's prototype handler
but that's probably desirable here anyway.

Add test cases covering this?

> Source/JavaScriptCore/API/JSObjectRef.h:343
> +When setting callAsConstructor it should be noted that it is not possible to
support JS subclassing via the extends clause of JS class syntax. This is
because there is no backwards compatible way to vend the derived class's
constructor to callAsConstructor. If you intend to subclass it's recomended to
use JSObjectMakeConstructor.

Spelling error: "recomended".

Not sure I understand the meaning of the phrase "backwards compatible" here.

The last sentence probably shouldn’t use the word "you". Maybe the sentence
should have a structure more like "JSObjectMakeConstructor <can do it>."

> Source/JavaScriptCore/API/tests/testapi.cpp:54
> +    APIString(String string)

I suggest const String&.

> Source/JavaScriptCore/API/tests/testapi.cpp:55
> +	   : APIString(string.ascii().data())

I suggest utf8() instead of ascii(), since ascii() is intended for debugging
only. Not great even for tests.

> Source/JavaScriptCore/API/tests/testapi.cpp:654
> +    JSClassDefinition definition = kJSClassDefinitionEmpty;

static const maybe?

> Source/JavaScriptCore/API/tests/testapi.cpp:660
> +    auto constructor = [] (JSContextRef ctx, JSObjectRef, size_t, const
JSValueRef*, JSValueRef*) -> JSObjectRef {
> +
> +	   return JSObjectMake(ctx, jsClass, nullptr);
> +    };

Stray blank line.

> Source/JavaScriptCore/API/tests/testapi.cpp:662
> +    JSObjectRef superClass = JSObjectMakeConstructor(context, jsClass,
constructor);

Superclass could be a single word instead of "super class".

> Source/JavaScriptCore/API/tests/testapi.cpp:664
> +    ScriptResult result = callFunction("(function (SuperClass) { class
SubClass extends SuperClass { method() { return 'value'; } }; return new
SubClass(); })", superClass);

Ditto. Superclass and subclass.

> Source/JavaScriptCore/API/tests/testapi.cpp:667
> +    JSObjectRef subClass = const_cast<JSObjectRef>(result.value());

Ditto.


More information about the webkit-reviews mailing list