[webkit-reviews] review granted: [Bug 213041] [JSC] JSCallbackObject::deleteProperty should redirect to Parent::deletePropertyByIndex if propertyName is index : [Attachment 401567] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki <ysuzuki at apple.com>'s
request for review:
Bug 213041: [JSC] JSCallbackObject::deleteProperty should redirect to
Parent::deletePropertyByIndex if propertyName is index
https://bugs.webkit.org/show_bug.cgi?id=213041

Attachment 401567: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 401567
  --> https://bugs.webkit.org/attachment.cgi?id=401567
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?