Bug 218697

Summary: [JSC] Support @@species in ArrayBuffer / SharedArrayBuffer slice
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: New BugsAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch
ews-feeder: commit-queue-
Patch ross.kirsling: review+

Description Yusuke Suzuki 2020-11-08 18:13:41 PST 
[JSC] Support @@species in ArrayBuffer / SharedArrayBuffer slice
Comment 1 Yusuke Suzuki 2020-11-08 18:16:48 PST 
Created attachment 413550 [details]
Patch
Comment 2 Yusuke Suzuki 2020-11-08 18:26:44 PST 
Created attachment 413551 [details]
Patch
Comment 3 Yusuke Suzuki 2020-11-08 18:37:40 PST 
Created attachment 413552 [details]
Patch
Comment 4 Yusuke Suzuki 2020-11-08 19:01:20 PST 
Created attachment 413553 [details]
Patch
Comment 5 Ross Kirsling 2020-11-08 19:53:16 PST 
Comment on attachment 413553 [details]
Patch

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

LGTM, based on my read of the spec & existing ArrayPrototype code and pending EWS.

> Source/JavaScriptCore/runtime/ArrayBufferSharingMode.h:33
>  enum class ArrayBufferSharingMode {

Should we mark this `: bool`?

> Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:59
> +enum class SpeciesConstructResult {

Should we mark this `: uint8_t`?

> Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:101
> +    // 16. Let new be ? Construct(ctor, « ð½(newLen) »).

It's kind of surprising that these `slice` checks have are all moved into speciesConstructArrayBuffer itself, but that may be fine as long as we're sure to link to the relevant spec sections beforehand:

https://tc39.es/ecma262/#sec-arraybuffer.prototype.slice
https://tc39.es/ecma262/#sec-sharedarraybuffer.prototype.slice
Comment 6 Yusuke Suzuki 2020-11-08 20:18:09 PST 
Comment on attachment 413553 [details]
Patch

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

Thanks!

>> Source/JavaScriptCore/runtime/ArrayBufferSharingMode.h:33
>>  enum class ArrayBufferSharingMode {
> 
> Should we mark this `: bool`?

Nice! I'll use `uint8_t` for now since we use this as an index in some places :)

>> Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:59
>> +enum class SpeciesConstructResult {
> 
> Should we mark this `: uint8_t`?

Nice, done.

>> Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:101
>> +    // 16. Let new be ? Construct(ctor, « ð½(newLen) »).
> 
> It's kind of surprising that these `slice` checks have are all moved into speciesConstructArrayBuffer itself, but that may be fine as long as we're sure to link to the relevant spec sections beforehand:
> 
> https://tc39.es/ecma262/#sec-arraybuffer.prototype.slice
> https://tc39.es/ecma262/#sec-sharedarraybuffer.prototype.slice

Nice! I'll put these links in this function :)
Comment 7 Yusuke Suzuki 2020-11-08 21:02:49 PST 
Committed r269574: <https://trac.webkit.org/changeset/269574>
Comment 8 Radar WebKit Bug Importer 2020-11-08 21:03:19 PST 
<rdar://problem/71175759>