Title: [216137] trunk
- Revision
- 216137
- Author
- keith_mil...@apple.com
- Date
- 2017-05-03 13:33:01 -0700 (Wed, 03 May 2017)
Log Message
Different behaviour with the .sort(callback) method (unlike Firefox & Chrome)
https://bugs.webkit.org/show_bug.cgi?id=47825
Reviewed by Saam Barati.
JSTests:
* stress/sorting-boolean-result-comparator.js: Added.
(checkArray):
Source/_javascript_Core:
This patch makes our sort function match the behavior of Firefox
and Chrome when the result of the comparison function is a
boolean. When we first switched to using merge sort, it regressed
JQuery sorting of DOM nodes by 30%. The regression was do to the
fact that JQuery was using compareDocumentPosition to compare the
locations of objects. Since one of the benchmarks would pass a
reverse sorted list to the sort function we would end up walking
the entire DOM to do comparisons. The solution to this was to
merge based on comparison(right, left) rather than
comparison(left, right). Although, in practice this does nothing
since sort could just as easily receive an already sorted list and
we're back in the same spot.
The downside of sorting with comparison(right, left) is that to
maintain stability when sorting, you only want to merge from right
when the comparison function returns a negative value. This is
where the problem with booleans comes in. Since booleans toNumber
false to 0 and true to 1 both values are "equal". This patch fixes
this by special casing boolean return values.
* builtins/ArrayPrototype.js:
(sort.merge):
LayoutTests:
Fix broken test.
* http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts-expected.txt:
Modified Paths
Added Paths
Diff
Modified: trunk/JSTests/ChangeLog (216136 => 216137)
--- trunk/JSTests/ChangeLog 2017-05-03 20:28:28 UTC (rev 216136)
+++ trunk/JSTests/ChangeLog 2017-05-03 20:33:01 UTC (rev 216137)
@@ -1,3 +1,13 @@
+2017-05-03 Keith Miller <keith_mil...@apple.com>
+
+ Different behaviour with the .sort(callback) method (unlike Firefox & Chrome)
+ https://bugs.webkit.org/show_bug.cgi?id=47825
+
+ Reviewed by Saam Barati.
+
+ * stress/sorting-boolean-result-comparator.js: Added.
+ (checkArray):
+
2017-05-02 David Kilzer <ddkil...@apple.com>
check-webkit-style should keep _javascript_ test functions in sync
Added: trunk/JSTests/stress/sorting-boolean-result-comparator.js (0 => 216137)
--- trunk/JSTests/stress/sorting-boolean-result-comparator.js (rev 0)
+++ trunk/JSTests/stress/sorting-boolean-result-comparator.js 2017-05-03 20:33:01 UTC (rev 216137)
@@ -0,0 +1,15 @@
+function checkArray(array) {
+ array = array.map((value, index) => { return { value, index }; });
+ array = array.sort((a, b) => b.value <= a.value);
+
+ for (let i = 1; i < array.length; i++) {
+ if (array[i].value < array[i - 1].value)
+ throw new Error();
+
+ if (array[i].value == array[i - 1].value && array[i].index <= array[i - 1].index)
+ throw new Error();
+ }
+}
+
+checkArray([7,4,2,0,5,5,4,3,9]);
+checkArray([1,0,1]);
Modified: trunk/LayoutTests/ChangeLog (216136 => 216137)
--- trunk/LayoutTests/ChangeLog 2017-05-03 20:28:28 UTC (rev 216136)
+++ trunk/LayoutTests/ChangeLog 2017-05-03 20:33:01 UTC (rev 216137)
@@ -1,3 +1,14 @@
+2017-05-03 Keith Miller <keith_mil...@apple.com>
+
+ Different behaviour with the .sort(callback) method (unlike Firefox & Chrome)
+ https://bugs.webkit.org/show_bug.cgi?id=47825
+
+ Reviewed by Saam Barati.
+
+ Fix broken test.
+
+ * http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts-expected.txt:
+
2017-05-03 Matt Lewis <jlew...@apple.com>
Mark http/tests/xmlhttprequest/supported-xml-content-types.html as flaky.
Modified: trunk/LayoutTests/http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts-expected.txt (216136 => 216137)
--- trunk/LayoutTests/http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts-expected.txt 2017-05-03 20:28:28 UTC (rev 216136)
+++ trunk/LayoutTests/http/tests/inspector/worker/blob-script-with-cross-domain-imported-scripts-expected.txt 2017-05-03 20:33:01 UTC (rev 216137)
@@ -8,7 +8,7 @@
http://localhost:8000/inspector/worker/resources/worker-blob-script.js
http://localhost:8000/inspector/worker/resources/worker-blob-import-script.js
SCRIPTS:
-blob:<sanitized>
http://localhost:8000/inspector/worker/resources/worker-blob-script.js
http://localhost:8000/inspector/worker/resources/worker-blob-import-script.js
+blob:<sanitized>
Modified: trunk/Source/_javascript_Core/ChangeLog (216136 => 216137)
--- trunk/Source/_javascript_Core/ChangeLog 2017-05-03 20:28:28 UTC (rev 216136)
+++ trunk/Source/_javascript_Core/ChangeLog 2017-05-03 20:33:01 UTC (rev 216137)
@@ -1,3 +1,34 @@
+2017-05-03 Keith Miller <keith_mil...@apple.com>
+
+ Different behaviour with the .sort(callback) method (unlike Firefox & Chrome)
+ https://bugs.webkit.org/show_bug.cgi?id=47825
+
+ Reviewed by Saam Barati.
+
+ This patch makes our sort function match the behavior of Firefox
+ and Chrome when the result of the comparison function is a
+ boolean. When we first switched to using merge sort, it regressed
+ JQuery sorting of DOM nodes by 30%. The regression was do to the
+ fact that JQuery was using compareDocumentPosition to compare the
+ locations of objects. Since one of the benchmarks would pass a
+ reverse sorted list to the sort function we would end up walking
+ the entire DOM to do comparisons. The solution to this was to
+ merge based on comparison(right, left) rather than
+ comparison(left, right). Although, in practice this does nothing
+ since sort could just as easily receive an already sorted list and
+ we're back in the same spot.
+
+ The downside of sorting with comparison(right, left) is that to
+ maintain stability when sorting, you only want to merge from right
+ when the comparison function returns a negative value. This is
+ where the problem with booleans comes in. Since booleans toNumber
+ false to 0 and true to 1 both values are "equal". This patch fixes
+ this by special casing boolean return values.
+
+
+ * builtins/ArrayPrototype.js:
+ (sort.merge):
+
2017-05-03 Andy VanWagoner <thetalecraf...@gmail.com>
[INTL] Support dashed values in unicode locale extensions
Modified: trunk/Source/_javascript_Core/builtins/ArrayPrototype.js (216136 => 216137)
--- trunk/Source/_javascript_Core/builtins/ArrayPrototype.js 2017-05-03 20:28:28 UTC (rev 216136)
+++ trunk/Source/_javascript_Core/builtins/ArrayPrototype.js 2017-05-03 20:33:01 UTC (rev 216137)
@@ -543,10 +543,17 @@
for (var dstIndex = left; dstIndex < rightEnd; ++dstIndex) {
if (right < rightEnd) {
- if (left >= leftEnd || comparator(src[right], src[left]) < 0) {
+ if (left >= leftEnd) {
dst[dstIndex] = src[right++];
continue;
}
+
+ let comparisonResult = comparator(src[right], src[left]);
+ if ((typeof comparisonResult === "boolean" && !comparisonResult) || comparisonResult < 0) {
+ dst[dstIndex] = src[right++];
+ continue;
+ }
+
}
dst[dstIndex] = src[left++];
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes
