Bug 158516 - Remove removeDirect
Summary: Remove removeDirect
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Barraclough
Depends on:
Reported: 2016-06-07 23:36 PDT by Gavin Barraclough
Modified: 2016-06-08 14:48 PDT (History)
9 users (show)

See Also:

Fix (11.24 KB, patch)
2016-06-07 23:42 PDT, Gavin Barraclough
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Barraclough 2016-06-07 23:36:23 PDT
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.
Comment 1 Gavin Barraclough 2016-06-07 23:42:43 PDT
Created attachment 280779 [details]
Comment 2 Ryosuke Niwa 2016-06-08 02:54:38 PDT
Comment on attachment 280779 [details]

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.


> 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;
Comment 3 Gavin Barraclough 2016-06-08 14:48:14 PDT
Committed revision 201834.