Bug 145365 – Array.of should work with other constructors

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Jordan Harband]

`function Foo(len) { this.length = len; } Array.of.apply(Foo, [1, 2, 3])` should return an instance of  `Foo` with the appropriate array index properties set, including "length".

[Jordan Harband]

Created attachment 253675 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 253675 [details]
Patch

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

> Source/JavaScriptCore/builtins/ArrayConstructor.js:27
> +  return Array.from.call(this, arguments);

Since Array.from looks up `@@iterator` of the given object, users can observe the spec violation by setting `arguments.__proto__[@@iterator]` getter.
I suggest just implementing the Array.of here instead of reusing Array.from.

And we need to pay attension to the references to the unsafe object (exposed to users).
In the above implementation, we refer `Array` and `Array.from`, `Array.from.call`. Both can be replaced by users so it's not safe.
(By overwriting `Array` / overwriting `Array.from` / overwriting `Array.from.call`.)

[Jordan Harband]

Created attachment 253789 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 253789 [details]
Patch

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

Great! Patch looks very nice to me (& nice catch!). Just nits.

> Source/JavaScriptCore/builtins/ArrayConstructor.js:26
> +function of(/* items... */) {

Please line break before `{`.

> Source/JavaScriptCore/builtins/ArrayConstructor.js:29
> +    var len = arguments.length;

These `len` / `A` / `C` are derived from the spec. But it is a little bit not readable for developers only looking this code.
Renaming more meaningful name is preferable.
`len` => `length`
`C` => `constructor`
`A` => `array`

> Source/JavaScriptCore/builtins/ArrayConstructor.js:31
> +    // TODO: Need isConstructor(thisObj) instead of typeof "function" check.

`isConstructor(thisObj)` is needed to fit to here's variable name.

> Source/JavaScriptCore/builtins/ArrayConstructor.js:34
> +    while (k < len) {

We can use for statement here. I know this `while` is corresponding to the sepc description. But for-statement is more readable ;)

for (var k = 0; k < length; ++k)

> Source/JavaScriptCore/builtins/ArrayConstructor.js:35
> +        @putByValDirect(A, @String(k), arguments[k])

`@String(k)` is not needed because Number => String conversion is not observable to users.
Just using `@putByValDirect(array, k, arguments[k])` is ok (and it is more efficient) ;)

> LayoutTests/js/script-tests/array-of.js:30
> +shouldBe("var foo = Array.of.call(Foo, 'a', 'b', 'c'); [foo.length, foo[0], foo[1], foo[2]]", "[3, 'a', 'b', 'c']");

Additionally, e would like to test the argument of `Foo`, according to the spec, `length` comes.

[Jordan Harband]

(In reply to comment #4)
> Comment on attachment 253789 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253789&action=review
> 
> Great! Patch looks very nice to me (& nice catch!). Just nits.
> 
> > Source/JavaScriptCore/builtins/ArrayConstructor.js:26
> > +function of(/* items... */) {
> 
> Please line break before `{`.
I'm following the same style as the "from" implementation in the file.

> 
> > Source/JavaScriptCore/builtins/ArrayConstructor.js:29
> > +    var len = arguments.length;
> 
> These `len` / `A` / `C` are derived from the spec. But it is a little bit
> not readable for developers only looking this code.
> Renaming more meaningful name is preferable.
> `len` => `length`
> `C` => `constructor`
> `A` => `array`
> 
> > Source/JavaScriptCore/builtins/ArrayConstructor.js:31
> > +    // TODO: Need isConstructor(thisObj) instead of typeof "function" check.
> 
> `isConstructor(thisObj)` is needed to fit to here's variable name.
> 
> > Source/JavaScriptCore/builtins/ArrayConstructor.js:34
> > +    while (k < len) {
> 
> We can use for statement here. I know this `while` is corresponding to the
> sepc description. But for-statement is more readable ;)
> 
> for (var k = 0; k < length; ++k)
> 
> > Source/JavaScriptCore/builtins/ArrayConstructor.js:35
> > +        @putByValDirect(A, @String(k), arguments[k])
> 
> `@String(k)` is not needed because Number => String conversion is not
> observable to users.
> Just using `@putByValDirect(array, k, arguments[k])` is ok (and it is more
> efficient) ;)
> 
> > LayoutTests/js/script-tests/array-of.js:30
> > +shouldBe("var foo = Array.of.call(Foo, 'a', 'b', 'c'); [foo.length, foo[0], foo[1], foo[2]]", "[3, 'a', 'b', 'c']");
> 
> Additionally, e would like to test the argument of `Foo`, according to the
> spec, `length` comes.

All the rest will be updated in the next patch.

[Darin Adler]

Comment on attachment 253789 [details]
Patch

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

>>> Source/JavaScriptCore/builtins/ArrayConstructor.js:26
>>> +function of(/* items... */) {
>> 
>> Please line break before `{`.
> 
> I'm following the same style as the "from" implementation in the file.

Sure, but you shouldn’t do that. Instead you should follow WebKit’s explicitly written style guidelines. We’ll fix that “from” function eventually.

[Darin Adler]

Comment on attachment 253789 [details]
Patch

So when you referred to the “next patch” does that mean there’s a new one coming to replace this one? If so, please clear the review and commit queue flags from this one.

[Jordan Harband]

Created attachment 253830 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 253830 [details]
Patch

r+

[WebKit Commit Bot]

Comment on attachment 253830 [details]
Patch

Clearing flags on attachment: 253830

Committed r184942: <http://trac.webkit.org/changeset/184942>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.