Bug 133898 – Store DOM constants directly in the JS object rather than jumping through a custom accessor

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

[Sam Weinig]

Store DOM constants directly in the JS object rather than jumping through a custom accessor

[Sam Weinig]

Created attachment 233105 [details]
Patch

[Sam Weinig]

There are two things I am not quite sure of with this patch:

1) Is the use of PropertySlot::setValue() for constants that still get accessed through the static lookup functions as efficient as PropertySlot::setCacheableCustom() with an accessor? If not, would it make sense to put the constant directly on the object when it is first looked up like we do with functions?  Should we do that with all properties now that we have CustomGetterSetter?

2) We have multiple variants of the static lookup functions to avoid unnecessary branches if a value of the specified type can't possibly be in the table. (e.g. constructors that only have constants used to use getStaticValueSlot(), but ones that had some functions as well had to use getStaticPropertySlot()).  I've added an extra branch to both getStaticValueSlot() and getStaticPropertySlot() but could have added new variants of them instead, and only called them the table had a constant.  This felt like overkill, but I could do it if anyone thinks that optimization is still important (or, I could add a single templatized version, where you just or in the set of possible types at compile time).

[Oliver Hunt]

Comment on attachment 233105 [details]
Patch

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

I

> Source/JavaScriptCore/ChangeLog:11
> +        and will make adding more flags possibles.

possible_s_ :)

> Source/WebCore/bindings/scripts/test/TestObj.idl:-248
> -    const DOMString CONST_VALUE_10 = "my constant string";

Why did we remove this? Do we just not have any non-integer constants in the dom?

[Sam Weinig]

(In reply to comment #3)
> (From update of attachment 233105 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=233105&action=review
> 
> I
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        and will make adding more flags possibles.
> 
> possible_s_ :)

Will fix.

> 
> > Source/WebCore/bindings/scripts/test/TestObj.idl:-248
> > -    const DOMString CONST_VALUE_10 = "my constant string";
> 
> Why did we remove this? Do we just not have any non-integer constants in the dom?

We both don't have any non-integer constants, and the WebIDL spec says it shouldn't be supported.

[Oliver Hunt]

Comment on attachment 233105 [details]
Patch

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

>>> Source/WebCore/bindings/scripts/test/TestObj.idl:-248
>>> -    const DOMString CONST_VALUE_10 = "my constant string";
>> 
>> Why did we remove this? Do we just not have any non-integer constants in the dom?
> 
> We both don't have any non-integer constants, and the WebIDL spec says it shouldn't be supported.

Well, I guess i'll let this change happen (but just this once) ;D

[Sam Weinig]

Committed revision 169979.