Bug 225032 – [JSC] Remove defaultValue() from the method table

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Alexey Shvayka]

[JSC] Remove defaultValue() from the method table

[Alexey Shvayka]

Created attachment 427005 [details]
Patch

[Darin Adler]

Comment on attachment 427005 [details]
Patch

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

> Source/JavaScriptCore/API/JSCallbackObject.h:202
> +    static EncodedJSValue customToPrimitiveImpl(JSGlobalObject*, CallFrame*);

Not sure what the word "impl" means in this function name. Aren’t most functions "implementations"? How is this one different?

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:235
> +    const JSCallbackObject* thisObject = jsDynamicCast<const JSCallbackObject*>(vm, callFrame->thisValue());

This is the kind of line of code where auto really shines, cutting down on repetitiveness and making this almost 1/3 shorter and easier to look over.

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:238
> +    PreferredPrimitiveType hint = toPreferredPrimitiveType(globalObject, callFrame->argument(0));

Same.

> Source/WebCore/bridge/runtime_object.cpp:187
> +        return setUpSymbolToPrimitiveSlot<convertRuntimeObjectToPrimitive>(thisObject, lexicalGlobalObject, slot);

We can hope that this Objective-C bridging is almost obsolete. Would be so nice to delete it some day.

The value materializing only at this point is not something I would necessarily be OK with if this was still in active use, but I suspect it will be good enough to keep existing clients working.

> Source/WebCore/bridge/runtime_object.cpp:228
> +    const RuntimeObject* thisObject = jsDynamicCast<const RuntimeObject*>(vm, callFrame->thisValue());

auto?

> Source/WebCore/bridge/runtime_object.cpp:231
> +    RefPtr<Instance> instance = thisObject->getInternalInstance();

auto and makeRefPtr?

> Source/WebCore/bridge/runtime_object.cpp:234
> +    PreferredPrimitiveType hint = toPreferredPrimitiveType(lexicalGlobalObject, callFrame->argument(0));

auto?

> Source/WebCore/bridge/runtime_object.h:94
> +template<RawNativeFunction toPrimitiveImpl>

Not a fan of the "impl" here.

> Source/WebCore/bridge/objc/objc_runtime.h:127
> +    ObjcInstance* getInternalObjCInstance() const { return _instance.get(); }

WebKit coding style recommends against the use of the word "get" in function names like this one.

> Source/WebCore/bridge/objc/objc_runtime.mm:314
> +    const ObjcFallbackObjectImp* thisObject = jsDynamicCast<const ObjcFallbackObjectImp*>(vm, callFrame->thisValue());

auto?

[Alexey Shvayka]

Created attachment 427013 [details]
Patch

Fix X86 build and npruntime tests, drop "impl" and lazy materialization, and use `auto` for WebCore.

[Alexey Shvayka]

(In reply to Darin Adler from comment #2)
> Comment on attachment 427005 [details]
> Patch

Thank you for detailed comments, Darin!

> > Source/JavaScriptCore/API/JSCallbackObject.h:202
> > +    static EncodedJSValue customToPrimitiveImpl(JSGlobalObject*, CallFrame*);
> 
> Not sure what the word "impl" means in this function name. Aren’t most
> functions "implementations"? How is this one different?

Removed the "impl".
It was there because customToPrimitive() has the similar signature to callImpl() / constructImpl(), but unlike them, it's private and self-contained.

> > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:235
> > +    const JSCallbackObject* thisObject = jsDynamicCast<const JSCallbackObject*>(vm, callFrame->thisValue());
> 
> This is the kind of line of code where auto really shines, cutting down on
> repetitiveness and making this almost 1/3 shorter and easier to look over.
> 
> > Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:238
> > +    PreferredPrimitiveType hint = toPreferredPrimitiveType(globalObject, callFrame->argument(0));
> 
> Same.

Removed the `const`, slightly cleaning up the code, but I am a little hesitant to use `auto` here.
Unlike WebCore, JSC code base is a bit more type-explicit: in this file, `auto` is used only for throw scopes.

> > Source/WebCore/bridge/runtime_object.cpp:187
> > +        return setUpSymbolToPrimitiveSlot<convertRuntimeObjectToPrimitive>(thisObject, lexicalGlobalObject, slot);
> 
> We can hope that this Objective-C bridging is almost obsolete. Would be so
> nice to delete it some day.
> 
> The value materializing only at this point is not something I would
> necessarily be OK with if this was still in active use, but I suspect it
> will be good enough to keep existing clients working.

The purpose of lazy materialization was to avoid creating & putting JSFunction during construction.
Since the bridging is almost obsolete, there is no point to (micro) optimize. Removed.

> > Source/WebCore/bridge/runtime_object.cpp:228
> > +    const RuntimeObject* thisObject = jsDynamicCast<const RuntimeObject*>(vm, callFrame->thisValue());
> 
> auto?
> 
> > Source/WebCore/bridge/runtime_object.cpp:231
> > +    RefPtr<Instance> instance = thisObject->getInternalInstance();
> 
> auto and makeRefPtr?
> 
> > Source/WebCore/bridge/runtime_object.cpp:234
> > +    PreferredPrimitiveType hint = toPreferredPrimitiveType(lexicalGlobalObject, callFrame->argument(0));
> 
> auto?

Changed.

> > Source/WebCore/bridge/runtime_object.h:94
> > +template<RawNativeFunction toPrimitiveImpl>
> 
> Not a fan of the "impl" here.

Removed the whole helper.

> > Source/WebCore/bridge/objc/objc_runtime.h:127
> > +    ObjcInstance* getInternalObjCInstance() const { return _instance.get(); }
> 
> WebKit coding style recommends against the use of the word "get" in function names like this one.

It does, but wouldn't it be nice to make ObjcFallbackObjectImp::getInternalObjCInstance() consistent with its friends?

    ObjCRuntimeObject::getInternalObjCInstance()
    CRuntimeObject::getInternalCInstance()
    RuntimeObject::getInternalInstance()

> > Source/WebCore/bridge/objc/objc_runtime.mm:314
> > +    const ObjcFallbackObjectImp* thisObject = jsDynamicCast<const ObjcFallbackObjectImp*>(vm, callFrame->thisValue());
> 
> auto?

Changed.

[Alexey Shvayka]

Created attachment 427040 [details]
Patch

Simplify RuntimeObject::getOwnPropertySlot() and tweak ChangeLog.

[Darin Adler]

Comment on attachment 427040 [details]
Patch

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

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:238
> +    JSCallbackObject* thisObject = jsDynamicCast<JSCallbackObject*>(vm, callFrame->thisValue());
> +    if (!thisObject)
> +        return throwVMTypeError(globalObject, scope, "JSCallbackObject[Symbol.toPrimitive] method called on incompatible |this| value."_s);
> +    PreferredPrimitiveType hint = toPreferredPrimitiveType(globalObject, callFrame->argument(0));

As someone who has been here with JavaScriptCore and WebCore all along, quite sad that the coding styles are diverging on minor stylistic questions like how we use auto.

[EWS]

Committed r276660 (237087@main): <https://commits.webkit.org/237087@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427040 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77228749>