jenkins-bot has submitted this change and it was merged.
Change subject: ve.utils: Fix bug in batchSplice polyfill
......................................................................
ve.utils: Fix bug in batchSplice polyfill
In order to test the polyfill, we have to check the support on
each call so it can be overridden. Cache the support check so
this isn't an expensive overhead.
Bonus: Fix return value of polyfill ('removed') by adding the offset
to the second argument.
Change-Id: I15f9e2fa88fc53d8af40bd76228e77a53cdf7672
---
M src/ve.utils.js
M tests/ve.test.js
2 files changed, 116 insertions(+), 89 deletions(-)
Approvals:
Krinkle: Looks good to me, approved
jenkins-bot: Verified
diff --git a/src/ve.utils.js b/src/ve.utils.js
index fe1380a..cf63d0e 100644
--- a/src/ve.utils.js
+++ b/src/ve.utils.js
@@ -174,6 +174,34 @@
ve.extendObject = $.extend;
/**
+ * @private
+ * @property {boolean}
+ */
+ve.supportSplice = ( function () {
+ var a, n;
+
+ // This returns false in Safari 8
+ a = new Array( 100000 );
+ a.splice( 30, 0, 'x' );
+ a.splice( 20, 1 );
+ if ( a.indexOf( 'x' ) !== 29 ) {
+ return false;
+ }
+
+ // This returns false in Opera 12.15
+ a = [];
+ n = 256;
+ a[n] = 'a';
+ a.splice( n + 1, 0, 'b' );
+ if ( a[n] !== 'a' ) {
+ return false;
+ }
+
+ // Splice is supported
+ return true;
+} )();
+
+/**
* Splice one array into another.
*
* This is the equivalent of arr.splice( offset, remove, d1, d2, d3, ... )
except that arguments are
@@ -183,7 +211,7 @@
* performance tests should be conducted on each use of this method to verify
this is true for the
* particular use. Also, browsers change fast, never assume anything, always
test everything.
*
- * Includes a replacement for broken implementation of
Array.prototype.splice() found in Opera 12.
+ * Includes a replacement for broken implementations of
Array.prototype.splice().
*
* @param {Array|ve.dm.BranchNode} arr Target object (must have `splice`
method, object will be modified)
* @param {number} offset Offset in arr to splice at. This may NOT be
negative, unlike the
@@ -192,80 +220,61 @@
* @param {Array} data Array of items to insert at the offset. Must be
non-empty if remove=0
* @return {Array} Array of items removed
*/
-ve.batchSplice = ( function () {
- var arraySplice;
+ve.batchSplice = function ( arr, offset, remove, data ) {
+ // We need to splice insertion in in batches, because of parameter list
length limits which vary
+ // cross-browser - 1024 seems to be a safe batch size on all browsers
+ var splice, spliced,
+ index = 0,
+ batchSize = 1024,
+ toRemove = remove,
+ removed = [];
- // This returns true in Opera 12.15
- function spliceBrokenOpera() {
- var n = 256,
- a = [];
- a[n] = 'a';
- a.splice( n + 1, 0, 'b' );
- return a[n] !== 'a';
- }
-
- // This returns true in Safari 8
- function spliceBrokenSafari() {
- var a = new Array( 100000 );
- a.splice( 30, 0, 'x' );
- a.splice( 20, 1 );
- return a.indexOf( 'x' ) !== 29;
- }
-
- if ( !spliceBrokenOpera() && !spliceBrokenSafari() ) {
- arraySplice = Array.prototype.splice;
+ if ( !Array.isArray( arr ) ) {
+ splice = arr.splice;
} else {
- // Standard Array.prototype.splice() function implemented using
.slice() and .push().
- arraySplice = function ( offset, remove/*, data... */ ) {
- var data, begin, removed, end;
+ if ( ve.supportSplice ) {
+ splice = Array.prototype.splice;
+ } else {
+ // Standard Array.prototype.splice() function
implemented using .slice() and .push().
+ splice = function ( offset, remove/*, data... */ ) {
+ var data, begin, removed, end;
- data = Array.prototype.slice.call( arguments, 2 );
+ data = Array.prototype.slice.call( arguments, 2
);
- begin = this.slice( 0, offset );
- removed = this.slice( offset, remove );
- end = this.slice( offset + remove );
+ begin = this.slice( 0, offset );
+ removed = this.slice( offset, offset + remove );
+ end = this.slice( offset + remove );
- this.length = 0;
- ve.batchPush( this, begin );
- ve.batchPush( this, data );
- ve.batchPush( this, end );
+ this.length = 0;
+ ve.batchPush( this, begin );
+ ve.batchPush( this, data );
+ ve.batchPush( this, end );
- return removed;
- };
+ return removed;
+ };
+ }
}
- return function ( arr, offset, remove, data ) {
- // We need to splice insertion in in batches, because of
parameter list length limits which vary
- // cross-browser - 1024 seems to be a safe batch size on all
browsers
- var splice, spliced,
- index = 0,
- batchSize = 1024,
- toRemove = remove,
- removed = [];
+ if ( data.length === 0 ) {
+ // Special case: data is empty, so we're just doing a removal
+ // The code below won't handle that properly, so we do it here
+ return splice.call( arr, offset, remove );
+ }
- splice = Array.isArray( arr ) ? arraySplice : arr.splice;
-
- if ( data.length === 0 ) {
- // Special case: data is empty, so we're just doing a
removal
- // The code below won't handle that properly, so we do
it here
- return splice.call( arr, offset, remove );
+ while ( index < data.length ) {
+ // Call arr.splice( offset, remove, i0, i1, i2, ..., i1023 );
+ // Only set remove on the first call, and set it to zero on
subsequent calls
+ spliced = splice.apply(
+ arr, [index + offset, toRemove].concat( data.slice(
index, index + batchSize ) )
+ );
+ if ( toRemove > 0 ) {
+ removed = spliced;
}
-
- while ( index < data.length ) {
- // Call arr.splice( offset, remove, i0, i1, i2, ...,
i1023 );
- // Only set remove on the first call, and set it to
zero on subsequent calls
- spliced = splice.apply(
- arr, [index + offset, toRemove].concat(
data.slice( index, index + batchSize ) )
- );
- if ( toRemove > 0 ) {
- removed = spliced;
- }
- index += batchSize;
- toRemove = 0;
- }
- return removed;
- };
-}() );
+ index += batchSize;
+ toRemove = 0;
+ }
+ return removed;
+};
/**
* Insert one array into another.
diff --git a/tests/ve.test.js b/tests/ve.test.js
index c125515..28529f7 100644
--- a/tests/ve.test.js
+++ b/tests/ve.test.js
@@ -266,35 +266,53 @@
);
} );
-QUnit.test( 'batchSplice', 8, function ( assert ) {
- var actualRet, expectedRet, i,
- actual = [ 'a', 'b', 'c', 'd', 'e' ],
- expected = actual.slice( 0 ),
- bigArr = [];
+QUnit.test( 'batchSplice', function ( assert ) {
+ var spliceWasSupported = ve.supportSplice;
- actualRet = ve.batchSplice( actual, 1, 1, [] );
- expectedRet = expected.splice( 1, 1 );
- assert.deepEqual( expectedRet, actualRet, 'removing 1 element (return
value)' );
- assert.deepEqual( expected, actual, 'removing 1 element (array)' );
+ function assertBatchSplice() {
+ var actualRet, expectedRet, msg, i,
+ actual = [ 'a', 'b', 'c', 'd', 'e' ],
+ expected = actual.slice( 0 ),
+ bigArr = [];
- actualRet = ve.batchSplice( actual, 3, 2, [ 'w', 'x', 'y', 'z' ] );
- expectedRet = expected.splice( 3, 2, 'w', 'x', 'y', 'z' );
- assert.deepEqual( expectedRet, actualRet, 'replacing 2 elements with 4
elements (return value)' );
- assert.deepEqual( expected, actual, 'replacing 2 elements with 4
elements (array)' );
+ msg = ve.supportSplice ? 'Array#splice native' : 'Array#splice
polyfill';
- actualRet = ve.batchSplice( actual, 0, 0, [ 'f', 'o', 'o' ] );
- expectedRet = expected.splice( 0, 0, 'f', 'o', 'o' );
- assert.deepEqual( expectedRet, actualRet, 'inserting 3 elements (return
value)' );
- assert.deepEqual( expected, actual, 'inserting 3 elements (array)' );
+ actualRet = ve.batchSplice( actual, 1, 1, [] );
+ expectedRet = expected.splice( 1, 1 );
+ assert.deepEqual( expectedRet, actualRet, msg + ': removing 1
element (return value)' );
+ assert.deepEqual( expected, actual, msg + ': removing 1 element
(array)' );
- for ( i = 0; i < 2100; i++ ) {
- bigArr[i] = i;
+ actualRet = ve.batchSplice( actual, 3, 2, [ 'w', 'x', 'y', 'z'
] );
+ expectedRet = expected.splice( 3, 2, 'w', 'x', 'y', 'z' );
+ assert.deepEqual( expectedRet, actualRet, msg + ': replacing 2
elements with 4 elements (return value)' );
+ assert.deepEqual( expected, actual, msg + ': replacing 2
elements with 4 elements (array)' );
+
+ actualRet = ve.batchSplice( actual, 0, 0, [ 'f', 'o', 'o' ] );
+ expectedRet = expected.splice( 0, 0, 'f', 'o', 'o' );
+ assert.deepEqual( expectedRet, actualRet, msg + ': inserting 3
elements (return value)' );
+ assert.deepEqual( expected, actual, msg + ': inserting 3
elements (array)' );
+
+ for ( i = 0; i < 2100; i++ ) {
+ bigArr[i] = i;
+ }
+ actualRet = ve.batchSplice( actual, 2, 3, bigArr );
+ expectedRet = expected.splice.apply( expected, [2, 3].concat(
bigArr.slice( 0, 1050 ) ) );
+ expected.splice.apply( expected, [1052, 0].concat(
bigArr.slice( 1050 ) ) );
+ assert.deepEqual( expectedRet, actualRet, msg + ': replacing 3
elements with 2100 elements (return value)' );
+ assert.deepEqual( expected, actual, msg + ': replacing 3
elements with 2100 elements (array)' );
}
- actualRet = ve.batchSplice( actual, 2, 3, bigArr );
- expectedRet = expected.splice.apply( expected, [2, 3].concat(
bigArr.slice( 0, 1050 ) ) );
- expected.splice.apply( expected, [1052, 0].concat( bigArr.slice( 1050 )
) );
- assert.deepEqual( expectedRet, actualRet, 'replacing 3 elements with
2100 elements (return value)' );
- assert.deepEqual( expected, actual, 'replacing 3 elements with 2100
elements (array)' );
+
+ QUnit.expect( 8 * ( spliceWasSupported ? 2 : 1 ) );
+
+ assertBatchSplice();
+
+ // If the current browser supported native splice,
+ // test again without the native splice.
+ if ( spliceWasSupported ) {
+ ve.supportSplice = false;
+ assertBatchSplice();
+ ve.supportSplice = true;
+ }
} );
QUnit.test( 'insertIntoArray', 3, function ( assert ) {
--
To view, visit https://gerrit.wikimedia.org/r/203054
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I15f9e2fa88fc53d8af40bd76228e77a53cdf7672
Gerrit-PatchSet: 4
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Esanders <esand...@wikimedia.org>
Gerrit-Reviewer: Catrope <roan.katt...@gmail.com>
Gerrit-Reviewer: Krinkle <krinklem...@gmail.com>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
