Bug 153081 – [ES6] Support subclassing Function.

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

[Keith Miller]

[ES6] Support subclassing Function.

[Keith Miller]

Created attachment 268908 [details]
Patch

[Geoffrey Garen]

Comment on attachment 268908 [details]
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:8
> +        This patch adds subclassing the Function object. It also, fixes an existing

No comma.

> Source/JavaScriptCore/ChangeLog:10
> +        the superclass' prototype property.

superclass's.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3230
> +        // FIXME: It's possible that these checks should be a single bytecode like: op_is_object_type.

This shouldn't be a FIXME. You should weight the tradeoffs now, and decide. If you decide this should change, you should file a bug.

I don't think it's very bad to have two branches here, since class construction is rare.

[Mark Lam]

Comment on attachment 268908 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        This patch adds subclassing the Function object. It also, fixes an existing
> 
> No comma.

missing "to" in "subclassing *to* the"?

> Source/JavaScriptCore/runtime/JSFunction.h:155
> -    JSFunction(VM&, FunctionExecutable*, JSScope*);
> +//    JSFunction(VM&, FunctionExecutable*, JSScope*);

Delete this instead of commenting out.

[Keith Miller]

Comment on attachment 268908 [details]
Patch

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

>>> Source/JavaScriptCore/ChangeLog:8
>>> +        This patch adds subclassing the Function object. It also, fixes an existing
>> 
>> No comma.
> 
> missing "to" in "subclassing *to* the"?

Fixed both issues. But I changed the first sentence to "This patch adds subclassing the Function object." => "This patch enables subclassing the Function object."

>> Source/JavaScriptCore/ChangeLog:10
>> +        the superclass' prototype property.
> 
> superclass's.

Fixed.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:3230
>> +        // FIXME: It's possible that these checks should be a single bytecode like: op_is_object_type.
> 
> This shouldn't be a FIXME. You should weight the tradeoffs now, and decide. If you decide this should change, you should file a bug.
> 
> I don't think it's very bad to have two branches here, since class construction is rare.

I agree that it's not too bad. Not only are class creations rare, but it's also rare to have the .prototype property be a function.

>> Source/JavaScriptCore/runtime/JSFunction.h:155
>> +//    JSFunction(VM&, FunctionExecutable*, JSScope*);
> 
> Delete this instead of commenting out.

Fixed.

[Keith Miller]

It looks like the test failures are just a missed rebaseline of class-syntax-extends.js. I will update before I land when the rest of the builders are done.

[Build Bot]

Comment on attachment 268908 [details]
Patch

Attachment 268908 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/688069

New failing tests:
js/class-syntax-extends.html

[Build Bot]

Created attachment 268915 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268908 [details]
Patch

Attachment 268908 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/688078

New failing tests:
js/class-syntax-extends.html

[Build Bot]

Created attachment 268917 [details]
Archive of layout-test-results from ews100 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 268908 [details]
Patch

Attachment 268908 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/688081

New failing tests:
js/class-syntax-extends.html

[Build Bot]

Created attachment 268919 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

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

[Keith Miller]

Committed r195070: <http://trac.webkit.org/changeset/195070>

[Ryan Haddad]

** The following JSC stress test failures have been introduced:
	es6.yaml/es6/Function_is_subclassable_Function.prototype.apply.js.default
	es6.yaml/es6/Function_is_subclassable_Function.prototype.call.js.default
	es6.yaml/es6/Function_is_subclassable_can_be_called.js.default
	es6.yaml/es6/Function_is_subclassable_can_be_used_with_new.js.default
	es6.yaml/es6/Function_is_subclassable_correct_prototype_chain.js.default

<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20JSC%20%28Tests%29/builds/2483>

[Keith Miller]

Fixing. Not sure why those didn't come up locally... I figured those ones would fail too but I should have checked they were marked as normal.

[Ryan Haddad]

(In reply to comment #14)
> Fixing. Not sure why those didn't come up locally... I figured those ones
> would fail too but I should have checked they were marked as normal.

Thank you!

[Keith Miller]

Tests should be fixed in https://trac.webkit.org/r195077