Bug 158516 – Remove removeDirect

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Gavin Barraclough]

removeDirect is typically used as a subroutine of deleteProperty, but is also available to call directly. Having this functionality factored out to a separate routine is a bad idea on a couple of fronts:

- for the main use within deleteProperty there is redundancy (presence of the property was being checked twice) and inconsistency (the two functions returned different results in the case of a nonexistent property; the result from removeDirect was never observed).

- all uses of removeDirect are in practical terms incorrect. removeDirect had the advantage of ignoring the configurable (DontDelete) attributes, but this is achievable using the DeletePropertyMode setting - and the disadvantage of failing delete static table properties. Last uses were one that was removed in bug #158295 (where failure to delete static properties was a problem), and as addressed in this patch removeDirect is being used to implement runtime enabled features. This only works because we currently force reification of all properties on the DOM prototype objects, so in effect there are no static properties. In order to make the code robust such that runtime enabled features would still work even if we were not reifying static properties (a change we may want to make) we should be calling deleteProperty in this case too.

[Gavin Barraclough]

Created attachment 280779 [details]
Fix

[Ryosuke Niwa]

Comment on attachment 280779 [details]
Fix

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

> Source/JavaScriptCore/runtime/JSObject.cpp:1506
> +                // Assert to guard assumption in above comment; either there is no property in property storage,

This comment seems redundant given the comment above & the assertion. Remove?

> Source/JavaScriptCore/runtime/JSObject.cpp:1518
> +    // Check if the property exists.
> +    if (isValidOffset(structure->get(vm, propertyName, attributes))) {

I think we usually prefer allocating a local variable with a descriptive name like:
bool propertyExists = isValidOffset(~)
instead of adding comment like this at least in WebCore.

> Source/JavaScriptCore/runtime/JSObject.cpp:1519
> +        // Return failure if the property is non-configurable (DontDelete).

This comment seems redundant.

> Source/JavaScriptCore/runtime/JSObject.cpp:1523
> +        // Remove the property from the structure.

Ditto.

> Source/JavaScriptCore/runtime/JSObject.cpp:1531
> +        // If there is a slot in property storage for this property, clear it for the benefit of GC.
> +        if (offset != invalidOffset)

I think this is another instance where a local variable with a descriptive name would help. e.g.
bool shouldClearPropertySlotForGC = offset != invalidOffset;

[Gavin Barraclough]

Committed revision 201834.