Bug 127523 – Generic JSObject::put should handle static properties in the classinfo hierarchy

• Component: New Bugs
• Status: RESOLVED FIXED

[Oliver Hunt]

Generic JSObject::put should handle static properties in the classinfo hierarchy

[Oliver Hunt]

Created attachment 222050 [details]
Patch

[Build Bot]

Comment on attachment 222050 [details]
Patch

Attachment 222050 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4899138265153536

New failing tests:
media/network-no-source-const-shadow.html
fast/canvas/webgl/oes-texture-half-float-with-canvas.html
fast/canvas/webgl/oes-texture-half-float-with-image.html
fast/canvas/webgl/oes-texture-half-float-linear.html
fast/canvas/webgl/oes-texture-half-float-with-video.html

[Build Bot]

Created attachment 222068 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Geoffrey Garen]

Comment on attachment 222050 [details]
Patch

Looks like you need to update bindings test expectations and fix some WebGL tests.

[Geoffrey Garen]

Comment on attachment 222050 [details]
Patch

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

Please add a regression test for a host object in the prototype chain with a setter -- for example, RegExp.input.

Your ChangeLog should specify how you tested performance. I suggest Dromaeo.

The rest of the patch looks good, with some comments below.

> Source/JavaScriptCore/create_hash_table:99
> +            if (!($att =~ m/ReadOnly/)) {
> +                $put = "set" . jsc_ucfirst($val);
> +            }
> +            $hasSetter = "true";

Why is $hasSetter true even if the property is ReadOnly? Is this because strict mode will need to throw an exception when setting a ReadOnly property? If so, I think you should call this variable "$hasSetterOrReadOnlyProperty".

> Source/JavaScriptCore/runtime/JSObject.cpp:398
> +        if (info->hasStaticSetterProperties(vm)) {

Same rename here.

> Source/JavaScriptCore/runtime/Lookup.h:113
> +        bool hasSetterProperties;

Same rename here.

[Oliver Hunt]

(In reply to comment #4)
> (From update of attachment 222050 [details])
> Looks like you need to update bindings test expectations and fix some WebGL tests.

Yeah, but i wanted to make the EWS bots try it out, to see how badly it horks everything.  Alas they have fallen over again.

Happily this is performance neutral (and actually a win on some tests!)

I agree with the rename.

[Oliver Hunt]

Created attachment 222127 [details]
Patch

[Geoffrey Garen]

Comment on attachment 222127 [details]
Patch

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

> Source/WebCore/ChangeLog:56
> +        * html/canvas/WebGLRenderingContext.idl:
> +          Remove bogus attribute

You should explain how you know this is bogus, so poor Dean Jackson doesn't have to wonder.

In person, you explained that this attribute never existed in the spec, and never should have been in WebKit.

[Geoffrey Garen]

Comment on attachment 222127 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:12
> +        To make this not clobber performance, the ClassInfo HashTable
> +        now includes a flag to indicate that it contains setters. This

You should specify the performance tests you ran: Dromaeo, and run-jsc-benchmarks.

[Oliver Hunt]

Committed r162713: <http://trac.webkit.org/changeset/162713>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 127593

[Oliver Hunt]

Committed r162740: <http://trac.webkit.org/changeset/162740>