Bug 133898

Summary: Store DOM constants directly in the JS object rather than jumping through a custom accessor
Product: WebKit Reporter: Sam Weinig <sam>
Component: New BugsAssignee: Sam Weinig <sam>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, cgarcia, commit-queue, esprehn+autocc, fpizlo, ggaren, kondapallykalyan, mhahnenberg, oliver
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch oliver: review+

Description Sam Weinig 2014-06-13 23:14:02 PDT
Store DOM constants directly in the JS object rather than jumping through a custom accessor
Comment 1 Sam Weinig 2014-06-13 23:31:08 PDT
Created attachment 233105 [details]
Patch
Comment 2 Sam Weinig 2014-06-13 23:45:01 PDT
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).
Comment 3 Oliver Hunt 2014-06-14 01:27:27 PDT
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?
Comment 4 Sam Weinig 2014-06-14 08:36:14 PDT
(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.
Comment 5 Oliver Hunt 2014-06-14 09:24:40 PDT
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
Comment 6 Sam Weinig 2014-06-14 13:39:18 PDT
Committed revision 169979.