Bug 148035 – [ES6] Add TypedArray.prototype functionality.

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

[Keith Miller]

This patch should add most of the functionality for the prototype properties of TypedArray objects in ES6. There are a few exceptions to this, which will be added in upcoming patches:

1) First we do not use the species constructor for some of the TypedArray prototype functions (namely: map, filter, slice, and subarray). That will need to be added when species constructors are finished.

2) TypedArrays still have a length, byteOffset, byteLength and buffer are still attached to the TypedArray instance (in the spec they are on the TypedArray.prototype instance object) since the JIT currently assumes those properties are fixed.

3) The TypedArray.constructor property is not added yet as it should point to the TypedArray instance object, which is added in a future patch.

[Keith Miller]

Created attachment 259030 [details]
Patch

[Sam Weinig]

Comment on attachment 259030 [details]
Patch

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

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:30
> +function every(callback /*, thisArg */) {
> +    "use strict";
> +    var length = @typedArrayLength(this);

I recently unified our style for the builtins to be:

function functionName(arguments)
{
    "use strict";

    rest of function goes here
}

It would be great if we could keep it consistent.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:98
> +    for (var i = 0; i < length; i++)
> +        if (callback.@call(thisArg, this[i], i, this))
> +            return true;

This for loop should have braces.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:343
> +    // This is matching FireFox behavior. In particular, it rejects all attempts to

This seems unrelated and I don't think it is correct.  Firefox is spelled as one word.

[Keith Miller]

Comment on attachment 259030 [details]
Patch

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

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:30
>> +    var length = @typedArrayLength(this);
> 
> I recently unified our style for the builtins to be:
> 
> function functionName(arguments)
> {
>     "use strict";
> 
>     rest of function goes here
> }
> 
> It would be great if we could keep it consistent.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:98
>> +            return true;
> 
> This for loop should have braces.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:343
>> +    // This is matching FireFox behavior. In particular, it rejects all attempts to
> 
> This seems unrelated and I don't think it is correct.  Firefox is spelled as one word.

Whoops, I was trying to figure out why that line was appearing in my diff but I couldn't figure it out... That would explain it.

[Keith Miller]

Created attachment 259051 [details]
Patch

Should build now. Also, addressed comments.

[Build Bot]

Comment on attachment 259051 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html

[Build Bot]

Created attachment 259053 [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

[Build Bot]

Comment on attachment 259051 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html
fast/dom/Window/window-postmessage-clone.html

[Build Bot]

Created attachment 259055 [details]
Archive of layout-test-results from ews101 for mac-mavericks

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

[Keith Miller]

Created attachment 259061 [details]
Patch

[Build Bot]

Comment on attachment 259061 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html

[Build Bot]

Created attachment 259065 [details]
Archive of layout-test-results from ews103 for mac-mavericks

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

[Build Bot]

Comment on attachment 259061 [details]
Patch

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

New failing tests:
fast/canvas/webgl/type-conversion-test.html

[Build Bot]

Created attachment 259069 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

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

[Keith Miller]

Created attachment 259166 [details]
Patch

[Keith Miller]

Created attachment 259195 [details]
Patch

[Geoffrey Garen]

Comment on attachment 259195 [details]
Patch

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

Looks good but needs some improvement.

> LayoutTests/ChangeLog:8
> +        Added tests for the TypedArray.prototype functions

functions => functions.

> LayoutTests/ChangeLog:9
> +        all the tests use the typedarray-test-helper-function.js

all => All

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:26
> +// Note that the intrisic @typedArrayLength checks the that the argument passed is a typed array.

Does it throw if not? You should say so.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:119
> +    function merge(dst, src, srcIndex, srcEnd, width, comparator)

Because typed arrays do not require stable sort (since their entries are all primitives), it would be nice to use a faster sorting algorithm, like quick sort. You should switch to quick sort in a follow-up patch.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:168
> +
> +        mergeSort(this, length, comparator);

Stray newline.

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:191
> +    else {

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:217
> +    else {

No braces for single-line block.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:238
> +    // FIXME: This should be a species constructor

constructor => constructor.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:266
> +    // FIXME: This should be a species constructor

constructor => constructor.

> Source/JavaScriptCore/builtins/TypedArray.prototype.js:269
> +    for (var i = 0; i < kept.length; i++) {

No braces for single-line block.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:89
> +

Stray newline.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:102
> +    TypeConverter* unionArray = bitwise_cast<TypeConverter*>(floatArray);

This.... probably doesn't do what you think it does.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:110
> +    // When we sort floating point numbers as ints comparing two negative numbers will
> +    // give the wrong ordering so we need to reverse these numbers.

Can you pass a comparator to std::sort that will do this naturally?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:225
> +

Stray newline.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:240
> +        default: {

No braces for single line statement.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:242
> +            }

It's good to break, even at the end.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:207
> +template<typename ViewClass>
> +EncodedJSValue JSC_HOST_CALL genericTypedArrayViewProtoFuncToLocaleString(ExecState* exec)

No JavaScript action on this one? It would help with the function call.

[Geoffrey Garen]

Comment on attachment 259195 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:102
>> +    TypeConverter* unionArray = bitwise_cast<TypeConverter*>(floatArray);
> 
> This.... probably doesn't do what you think it does.

To clarify here, our strategy is to read the array as if it were a union, to avoid strict aliasing violation. That means the initial cast of the pointer to first member can be reinterpret_cast, and all subsequent access should be through the union type, which means we should not bitwise cast below, and we should sort an array of union and not an array of int.

[Yusuke Suzuki]

Comment on attachment 259195 [details]
Patch

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

Great, significant work :D

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:87
> +void sortFloat(FloatType* floatArray, unsigned length)

I suggest you to write why this function works.
Or I suggest you to write this as a simple std::sort at first (and later, optimize it with the benchmark :D)

(the following one is my rough understanding),

IEEE 754 ensures that the mantissa and exponential part of the format (63bit) is ordered except for NaN.
Because IEEE 754 does not use complement representation for the negative values, the negative float numbers recognized as the integral numbers are reverse ordered.
We summarize the result of the comparison between the floating numbers recognized as the integral ones.

- <=> - = reversed (-30 > -20 = true)
+ <=> + = ordered (30 > 20 = true)
- <=> + = ordered (-30 > 20 = false)
+ <=> - = ordered (30 > -20 = true)

As a result, when sorting the array,
1. unsigned numbers are correctly ordered.
2. The order of unsigned and signed numbers are correctly kept.
3. But the order between signed numbers are reversed.



For NaN, we normalize the NaN to the specific representation; sign bit is 0, all exponential part is 1 and the MSB of the mantissa is 1.
So NaN is recognized as the largest integral numbers.

I suggest you to test with the following edge numbers,

1. positive infinity, negative infinity
2. -0.0, +0.0
3. NaN
4. denormalized numbers

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:104
> +        unionArray[i].floatType = purifyNaN(unionArray[i].floatType);

I think, here, we can just write

for (unsigned i = 0; i < length; ++i)
   floatArray[i] = purifyNaN(floatArray[i]);

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:106
> +    IntegralType* intArray = bitwise_cast<IntegralType*>(unionArray);

Can we just write `bitwise_cast<IntegralType*>(floatArray)`?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:112
> +    while (lastNegativeIndex < length && std::signbit(unionArray[++lastNegativeIndex].floatType)) { }

Let's replace `{ }` to `;`.
I think we can check it by using intArray (intArray[i] < 0) instead of using `std::signbit`.

And this code has the issue of overrun. When `length == 1`, this will access `unionArray[1]`.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:108
> +    long final = argumentClampedIndexFromStartOrEnd(exec, 2, length, length);

Why are they `long`?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:182
> +    StringView separator;

StringView is just the view and it does not have any ownership of the held string value.
So when using it, we need to take care of the lifetime of the owner string.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:185
> +        separator = { &comma, 1 };

Is this &comma addressing safe? This comma address is the address of the stack.
When this brace is ended, the lifetime of the variable `comma` is finished.
I think we need to change it to the following.

const LChar* comma = ",";
separator = { comma, 1 };

String Literal is allocated in the static area and its lifetime is the same to the program.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
> +        separator = separatorString->view(exec);

If we would like to keep separator valid, we need to maintain separatorString alive.
I suggest you to move `JSString* separatorString` to the function top-level block to make it explicit that separatorString is alive during this function.

> Source/JavaScriptCore/runtime/JSObject.h:1492
> +    } while (false)

Nice!

> Source/JavaScriptCore/runtime/JSTypedArrayPrototypes.cpp:35
> +    CREATE_METHOD_TABLE(JSTypedArrayViewPrototype)};

We can just write `&Base::s_info` here.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:69
> +        return throwVMError(exec, createTypeError(exec, "Receiver should be a typed array view"));

Could you add "Receiver should be a typed array view but was not an object" test cases for each function?

[Keith Miller]

Comment on attachment 259195 [details]
Patch

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

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:26
>> +// Note that the intrisic @typedArrayLength checks the that the argument passed is a typed array.
> 
> Does it throw if not? You should say so.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:168
>> +        mergeSort(this, length, comparator);
> 
> Stray newline.
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:191
>> +    else {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:217
>> +    else {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:238
>> +    // FIXME: This should be a species constructor
> 
> constructor => constructor.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:266
>> +    // FIXME: This should be a species constructor
> 
> constructor => constructor.

Fixed.

>> Source/JavaScriptCore/builtins/TypedArray.prototype.js:269
>> +    for (var i = 0; i < kept.length; i++) {
> 
> No braces for single-line block.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:87
>> +void sortFloat(FloatType* floatArray, unsigned length)
> 
> I suggest you to write why this function works.
> Or I suggest you to write this as a simple std::sort at first (and later, optimize it with the benchmark :D)
> 
> (the following one is my rough understanding),
> 
> IEEE 754 ensures that the mantissa and exponential part of the format (63bit) is ordered except for NaN.
> Because IEEE 754 does not use complement representation for the negative values, the negative float numbers recognized as the integral numbers are reverse ordered.
> We summarize the result of the comparison between the floating numbers recognized as the integral ones.
> 
> - <=> - = reversed (-30 > -20 = true)
> + <=> + = ordered (30 > 20 = true)
> - <=> + = ordered (-30 > 20 = false)
> + <=> - = ordered (30 > -20 = true)
> 
> As a result, when sorting the array,
> 1. unsigned numbers are correctly ordered.
> 2. The order of unsigned and signed numbers are correctly kept.
> 3. But the order between signed numbers are reversed.
> 
> 
> 
> For NaN, we normalize the NaN to the specific representation; sign bit is 0, all exponential part is 1 and the MSB of the mantissa is 1.
> So NaN is recognized as the largest integral numbers.
> 
> I suggest you to test with the following edge numbers,
> 
> 1. positive infinity, negative infinity
> 2. -0.0, +0.0
> 3. NaN
> 4. denormalized numbers

Added Comment

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:89
>> +
> 
> Stray newline.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:225
>> +
> 
> Stray newline.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:240
>> +        default: {
> 
> No braces for single line statement.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayView.h:242
>> +            }
> 
> It's good to break, even at the end.

Fixed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:108
>> +    long final = argumentClampedIndexFromStartOrEnd(exec, 2, length, length);
> 
> Why are they `long`?

I made them long so no overflow would occur in computations for count.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:185
>> +        separator = { &comma, 1 };
> 
> Is this &comma addressing safe? This comma address is the address of the stack.
> When this brace is ended, the lifetime of the variable `comma` is finished.
> I think we need to change it to the following.
> 
> const LChar* comma = ",";
> separator = { comma, 1 };
> 
> String Literal is allocated in the static area and its lifetime is the same to the program.

I believe you are correct. I just copied this code from ArrayPrototype.cpp and didn't give it too much thought. We should probably fix this there as well.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:190
>> +        separator = separatorString->view(exec);
> 
> If we would like to keep separator valid, we need to maintain separatorString alive.
> I suggest you to move `JSString* separatorString` to the function top-level block to make it explicit that separatorString is alive during this function.

I don't think there is an issue here. I believe JSString* lives in the js heap so it needs to be marked before it can be GCed.

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewPrototypeFunctions.h:207
>> +EncodedJSValue JSC_HOST_CALL genericTypedArrayViewProtoFuncToLocaleString(ExecState* exec)
> 
> No JavaScript action on this one? It would help with the function call.

Yeah, you are probably right. I moved this to a builtin.

>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:69
>> +        return throwVMError(exec, createTypeError(exec, "Receiver should be a typed array view"));
> 
> Could you add "Receiver should be a typed array view but was not an object" test cases for each function?

Fixed.

[Keith Miller]

Created attachment 259381 [details]
Patch

[Keith Miller]

Created attachment 259425 [details]
Patch

Per offline discussion, I realized that it is necessary to maintain the JSString* throughout the lifetime of Join.

[Keith Miller]

Created attachment 259427 [details]
Patch

Uploaded the wrong file...

[Geoffrey Garen]

Comment on attachment 259427 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:37
> +#define CALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTION(functionName) do {                           \

You should just use an inline function here. It's easier to debug and not any slower.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:65
> +static EncodedJSValue JSC_HOST_CALL typedArrayViewPrivateFuncLength(ExecState* exec)

Why are all of these functions in an Inlines.h file? It seems that we never inline them.

I think all of this stuff belongs in a .cpp file somewhere.

[Keith Miller]

Comment on attachment 259427 [details]
Patch

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

>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:37
>> +#define CALL_GENERIC_TYPEDARRAY_PROTOTYPE_FUNCTION(functionName) do {                           \
> 
> You should just use an inline function here. It's easier to debug and not any slower.

Unfortunately, I don't think that's possible as the function is a templated thing and C++ can't take templated functions as parameters. At least as far as I know.

>> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototypeInlines.h:65
>> +static EncodedJSValue JSC_HOST_CALL typedArrayViewPrivateFuncLength(ExecState* exec)
> 
> Why are all of these functions in an Inlines.h file? It seems that we never inline them.
> 
> I think all of this stuff belongs in a .cpp file somewhere.

That was an oversight on my part. At one point I had some template functions in here but when I removed them I forgot to move everything to a .cpp file. It's fixed now.

[Keith Miller]

Created attachment 259684 [details]
Patch

[Keith Miller]

Created attachment 259773 [details]
Patch

Trunk looked broken trying again.

[Keith Miller]

Created attachment 259979 [details]
Patch

[Keith Miller]

Created attachment 259996 [details]
Patch

[Geoffrey Garen]

Comment on attachment 259996 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 259996 [details]
Patch

Clearing flags on attachment: 259996

Committed r189064: <http://trac.webkit.org/changeset/189064>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Jordan Harband]

This is awesome! What about Array#includes and TypedArray#includes? These are stage 3 in ES7, and as such are ready for browser implementations.

[Brent Fulgham]

This broke the Windows build.

[Brent Fulgham]

Missing symbols:

     Creating library C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.lib and object C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\lib32\JavaScriptCore.exp
JSGlobalObject.obj : error LNK2019: unresolved external symbol "public: static class JSC::JSTypedArrayViewPrototype * __cdecl JSC::JSTypedArrayViewPrototype::create(class JSC::VM &,class JSC::JSGlobalObject *,class JSC::Structure *)" (?create@JSTypedArrayViewPrototype@JSC@@SAPAV12@AAVVM@2@PAVJSGlobalObject@2@PAVStructure@2@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
JSGlobalObject.obj : error LNK2019: unresolved external symbol "public: static class JSC::Structure * __cdecl JSC::JSTypedArrayViewPrototype::createStructure(class JSC::VM &,class JSC::JSGlobalObject *,class JSC::JSValue)" (?createStructure@JSTypedArrayViewPrototype@JSC@@SAPAVStructure@2@AAVVM@2@PAVJSGlobalObject@2@VJSValue@2@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
JSGlobalObject.obj : error LNK2019: unresolved external symbol "__int64 __fastcall JSC::typedArrayViewPrivateFuncSort(class JSC::ExecState *)" (?typedArrayViewPrivateFuncSort@JSC@@YI_JPAVExecState@1@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
JSGlobalObject.obj : error LNK2019: unresolved external symbol "__int64 __fastcall JSC::typedArrayViewPrivateFuncLength(class JSC::ExecState *)" (?typedArrayViewPrivateFuncLength@JSC@@YI_JPAVExecState@1@@Z) referenced in function "private: void __thiscall JSC::JSGlobalObject::init(class JSC::VM &)" (?init@JSGlobalObject@JSC@@AAEXAAVVM@2@@Z) [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\bin32\JavaScriptCore.dll : fatal error LNK1120: 4 unresolved externals [C:\cygwin\home\buildbot\WebKit\Source\JavaScriptCore\JavaScriptCore.vcxproj\JavaScriptCore.vcxproj]

[Filip Pizlo]

This broke JSC stress testing because of the typedarray-test-helper-function.js file, which the stress test harness thinks is a test.

[Csaba Osztrogonác]

(In reply to comment #35)
> This broke JSC stress testing because of the
> typedarray-test-helper-function.js file, which the stress test harness
> thinks is a test.

Mark already fixed it in https://trac.webkit.org/changeset/189084 .

But the Windows build is still broken ... Otherwise why did 
you land it without running tests and fixing Windows build?

[WebKit Commit Bot]

Re-opened since this is blocked by bug 148560

[Mark Lam]

FYI, rolled out in r189085: <http://trac.webkit.org/changeset/189085>

[Keith Miller]

Sorry, about the issues. I didn't realize the javascript tests were not run on EWS. I'll roll out the patch and fix theses things

[Csaba Osztrogonác]

(In reply to comment #38)
> FYI, rolled out in r189085: <http://trac.webkit.org/changeset/189085>

Brent landed a Windows buildfix in https://trac.webkit.org/changeset/189077
(necessary, but not enough) and I reverted it in https://trac.webkit.org/changeset/189090 to fix the Windows build after the rollout.

[Csaba Osztrogonác]

Brent landed one more buildfix - https://trac.webkit.org/changeset/189072
I reverted the part of r189072 which is responsible for fixing this bug:
https://trac.webkit.org/changeset/189091

[Keith Miller]

Created attachment 260315 [details]
Patch

Moved tests out of LayoutTests and into JavascriptCore/tests/stress which should fix the testing problems. Hopefully, the Windows build is fixed but I'll check the EWS results.

[Keith Miller]

Windows is still not building. I'll have to find a Windows machine to debug it.

[Yusuke Suzuki]

Comment on attachment 260315 [details]
Patch

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

You need to add introduced *.cpp and *.h to JavaScriptCore.vcxproj/JavaScriptCore.vcxproj and JavaScriptCore.vcxproj/JavaScriptCore.vcxproj.filters to build sources in Windows.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:69
> +EncodedJSValue typedArrayViewPrivateFuncLength(ExecState* exec)

Let's add JSC_HOST_CALL annotation. It enforces __fastcall calling convention in Windows environment.

> Source/JavaScriptCore/runtime/JSTypedArrayViewPrototype.cpp:78
> +EncodedJSValue typedArrayViewPrivateFuncSort(ExecState* exec)

Let's add JSC_HOST_CALL annotation. It enforces __fastcall calling convention in Windows environment.

[Keith Miller]

Created attachment 260533 [details]
Patch

Fixed the Windows build for real this time... hopefully.

[Keith Miller]

Welp, things are still broken... On the plus side it doesn't look like it's my changes. I'll try running again later

[Keith Miller]

Created attachment 260608 [details]
Patch

Running windows build again since the last time the error looked unrelated

[Keith Miller]

Created attachment 260614 [details]
Patch

Other people shouldn't be allowed to add files to Xcode...

[WebKit Commit Bot]

Attachment 260614 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/ChangeLog:27:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
ERROR: LayoutTests/ChangeLog:19:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Keith Miller]

Created attachment 260621 [details]
Patch

Didn't merge ChangeLogs Correctly.

[Geoffrey Garen]

Comment on attachment 260621 [details]
Patch

r=me

[Keith Miller]

Committed r190367: <http://trac.webkit.org/changeset/190367>

[Brent Fulgham]

This seems to have introduced a build failure on Windows. Are these new symbols?

C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(219): error C2065: 'typedArrayPrototypeEveryCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(220): error C2065: 'typedArrayPrototypeFilterCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(221): error C2065: 'typedArrayPrototypeSortCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(224): error C2065: 'typedArrayPrototypeFindCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(225): error C2065: 'typedArrayPrototypeFindIndexCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(226): error C2065: 'typedArrayPrototypeForEachCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(232): error C2065: 'typedArrayPrototypeMapCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(233): error C2065: 'typedArrayPrototypeReduceCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(234): error C2065: 'typedArrayPrototypeReduceRightCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(238): error C2065: 'typedArrayPrototypeSomeCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]
C:\cygwin\home\buildbot\slave\win-release\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(240): error C2065: 'typedArrayPrototypeToLocaleStringCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-release\build\WebKitBuild\Release\Source\JavaScriptCore\JavaScriptCore.vcxproj]

[Keith Miller]

I was hoping that the fact that it merged cleanly meant that it would still build. Fixing

[Keith Miller]

Windows build should be fixed with http://trac.webkit.org/changeset/190373

[Simon Fraser (smfr)]

Windows build is still failing. I'm going to roll out so that you can fix Windows offline.

  JSTypedArrayViewPrototype.cpp
C:\cygwin\home\buildbot\slave\win-debug\build\Source\JavaScriptCore\runtime\JSTypedArrayViewPrototype.cpp(219): error C2065: 'typedArrayPrototypeEveryCodeGenerator': undeclared identifier [C:\cygwin\home\buildbot\slave\win-debug\build\WebKitBuild\Debug\Source\JavaScriptCore\JavaScriptCore.vcxproj]

[WebKit Commit Bot]

Re-opened since this is blocked by bug 149694

[Keith Miller]

Committed r190429: <http://trac.webkit.org/changeset/190429>

[Alexey Shvayka]

*** Bug 138218 has been marked as a duplicate of this bug. ***