Bug 213041 – [JSC] JSCallbackObject::deleteProperty should redirect to Parent::deletePropertyByIndex if propertyName is index

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Yusuke Suzuki]

[JSC] JSCallbackObject::deleteProperty should redirect to Parent::deletePropertyByIndex if propertyName is index

[Yusuke Suzuki]

Created attachment 401567 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/64204300>

[Darin Adler]

Comment on attachment 401567 [details]
Patch

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

> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:424
> +    static_assert(std::is_final_v<JSCallbackObject<Parent>>, "Ensure no derived classes have custom deletePropertyByIndex implementation");
> +    if (Optional<uint32_t> index = parseIndex(propertyName))
> +        return Parent::deletePropertyByIndex(thisObject, globalObject, index.value());
>      return Parent::deleteProperty(thisObject, globalObject, propertyName, slot);

Seems like we have a small design problem here.

Here we call parseIndex and determine something is not an index, but then call through to the parent without transmitting that information. Then, it also has to call parseIndex.

Seems like there should be one function that doesn’t know if the property name is an index or not, another one that knows it’s an index, and a third that knows it’s not an index.

Wouldn’t we like to make a design so that parsing to see if a property name is an index happens exactly once?

[Yusuke Suzuki]

Comment on attachment 401567 [details]
Patch

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

>> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:424
>>      return Parent::deleteProperty(thisObject, globalObject, propertyName, slot);
> 
> Seems like we have a small design problem here.
> 
> Here we call parseIndex and determine something is not an index, but then call through to the parent without transmitting that information. Then, it also has to call parseIndex.
> 
> Seems like there should be one function that doesn’t know if the property name is an index or not, another one that knows it’s an index, and a third that knows it’s not an index.
> 
> Wouldn’t we like to make a design so that parsing to see if a property name is an index happens exactly once?

Yeah, agree. I think we eventually need to refactor these functions. Currently, deleteProperty accepts index and non-index. We should consider separating this into two functions.
However, we need to be extra careful when doing that, otherwise, we accidentally introduce additional indirect calls which do not exist before.

[Darin Adler]

Comment on attachment 401567 [details]
Patch

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

>>> Source/JavaScriptCore/API/JSCallbackObjectFunctions.h:424
>>>      return Parent::deleteProperty(thisObject, globalObject, propertyName, slot);
>> 
>> Seems like we have a small design problem here.
>> 
>> Here we call parseIndex and determine something is not an index, but then call through to the parent without transmitting that information. Then, it also has to call parseIndex.
>> 
>> Seems like there should be one function that doesn’t know if the property name is an index or not, another one that knows it’s an index, and a third that knows it’s not an index.
>> 
>> Wouldn’t we like to make a design so that parsing to see if a property name is an index happens exactly once?
> 
> Yeah, agree. I think we eventually need to refactor these functions. Currently, deleteProperty accepts index and non-index. We should consider separating this into two functions.
> However, we need to be extra careful when doing that, otherwise, we accidentally introduce additional indirect calls which do not exist before.

I agree with you, too. There’s danger of too many levels of function calls, other types of repeated work, and poor performance if this isn’t done right. It also offends my sensibilities to parse the same string twice.

[Yusuke Suzuki]

In EWS, only imported/w3c/web-platform-tests/css/css-grid/grid-items/grid-items-minimum-height-orthogonal-001.html is failing, but we know that this is failing without this patch. Landing.

[EWS]

Committed r262872: <https://trac.webkit.org/changeset/262872>

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