Bug 142707 - ES7: Implement Array.prototype.includes
Summary: ES7: Implement Array.prototype.includes
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dean Jackson
Keywords: InRadar
Depends on:
Reported: 2015-03-14 21:39 PDT by Dean Jackson
Modified: 2015-03-23 14:38 PDT (History)
3 users (show)

See Also:

Patch (7.55 KB, patch)
2015-03-14 21:43 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (560.04 KB, application/zip)
2015-03-14 22:18 PDT, Build Bot
no flags Details
Patch (11.63 KB, patch)
2015-03-14 22:19 PDT, Dean Jackson
no flags Details | Formatted Diff | Diff
Patch (11.54 KB, patch)
2015-03-22 15:21 PDT, Dean Jackson
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dean Jackson 2015-03-14 21:39:10 PDT
ES7: Implement Array.prototype.includes
Comment 1 Radar WebKit Bug Importer 2015-03-14 21:43:02 PDT
Comment 2 Dean Jackson 2015-03-14 21:43:13 PDT
Created attachment 248667 [details]
Comment 3 Build Bot 2015-03-14 22:18:09 PDT
Comment on attachment 248667 [details]

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

New failing tests:
Comment 4 Build Bot 2015-03-14 22:18:12 PDT
Created attachment 248671 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5
Comment 5 Dean Jackson 2015-03-14 22:19:53 PDT
Created attachment 248672 [details]
Comment 6 Geoffrey Garen 2015-03-16 10:38:00 PDT
Comment on attachment 248672 [details]

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

r- because I found a missing exception check.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1101
> +EncodedJSValue JSC_HOST_CALL arrayProtoFuncIncludes(ExecState* exec)

Are you scarred enough from writing of in JavaScript that you chose C++ for this? :)

Seriously, though, JS might be better.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1104
> +    JSObject* thisObj = exec->thisValue().toThis(exec, StrictMode).toObject(exec);

I think that toObject can throw, in which case getLength below will has a sad. So, you need to check for exception before getLength and after. Please add a test for this. I think all you need is to pass in undefined or null as 'this'.

This is the kind of thing that is easier to get right if you write it in JS.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1112
> +        JSValue e = getProperty(exec, thisObj, index);

It's a bit of a shame that this access is always virtual, with no fast path for arrays. This kind of thing sometimes goes faster in the JIT, since it will specialize for arrays if that's what the website uses.
Comment 7 Dean Jackson 2015-03-22 15:21:02 PDT
Created attachment 249208 [details]
Comment 8 Geoffrey Garen 2015-03-23 10:55:32 PDT
Comment on attachment 249208 [details]

Comment 9 Dean Jackson 2015-03-23 14:38:38 PDT
Committed r181871: <http://trac.webkit.org/changeset/181871>