Bug 153081 - [ES6] Support subclassing Function.
Summary: [ES6] Support subclassing Function.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords:
Depends on:
Blocks: 142591
  Show dependency treegraph
 
Reported: 2016-01-13 15:01 PST by Keith Miller
Modified: 2016-01-14 14:45 PST (History)
7 users (show)

See Also:


Attachments
Patch (13.07 KB, patch)
2016-01-13 15:50 PST, Keith Miller
ggaren: review+
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews117 for mac-yosemite (805.18 KB, application/zip)
2016-01-13 16:52 PST, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-yosemite (887.41 KB, application/zip)
2016-01-13 16:53 PST, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (801.09 KB, application/zip)
2016-01-13 16:58 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2016-01-13 15:01:16 PST
[ES6] Support subclassing Function.
Comment 1 Keith Miller 2016-01-13 15:50:32 PST
Created attachment 268908 [details]
Patch
Comment 2 Geoffrey Garen 2016-01-13 16:07:07 PST
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.
Comment 3 Mark Lam 2016-01-13 16:08:15 PST
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.
Comment 4 Keith Miller 2016-01-13 16:43:45 PST
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.
Comment 5 Keith Miller 2016-01-13 16:45:14 PST
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.
Comment 6 Build Bot 2016-01-13 16:52:29 PST
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
Comment 7 Build Bot 2016-01-13 16:52:31 PST
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
Comment 8 Build Bot 2016-01-13 16:53:18 PST
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
Comment 9 Build Bot 2016-01-13 16:53:20 PST
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
Comment 10 Build Bot 2016-01-13 16:58:04 PST
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
Comment 11 Build Bot 2016-01-13 16:58:05 PST
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
Comment 12 Keith Miller 2016-01-14 12:46:02 PST
Committed r195070: <http://trac.webkit.org/changeset/195070>
Comment 13 Ryan Haddad 2016-01-14 13:40:43 PST
** 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>
Comment 14 Keith Miller 2016-01-14 13:43:02 PST
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.
Comment 15 Ryan Haddad 2016-01-14 13:50:19 PST
(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!
Comment 16 Keith Miller 2016-01-14 14:45:35 PST
Tests should be fixed in https://trac.webkit.org/r195077