From 9a9b930a962c34d4a6f52ea24b044def84afc40c Mon Sep 17 00:00:00 2001 From: Timo Tijhof Date: Fri, 20 Nov 2015 01:14:09 +0000 Subject: [PATCH] resourceloader: Optimise mw.loader.register() * Remove redundant 'len' variables. This is a dated micro-optimisation that is no longer useful and can actually have an adverse effect. * Remove [.length] assignment. Use native push() instead. * Remove JSLint-ism of "i += 1" instead of "i++". * Remove redundant 'delete' operation for local 'unresolved' object. This falls naturally out of scope. Mutating this object to save memory probably wastes more cycles than it saves. Change-Id: Ib3cb40d2fef696078bd64585db9498326f08b2d2 --- resources/src/mediawiki/mediawiki.js | 52 +++++++++---------- .../resources/mediawiki/mediawiki.test.js | 17 ++++-- 2 files changed, 39 insertions(+), 30 deletions(-) diff --git a/resources/src/mediawiki/mediawiki.js b/resources/src/mediawiki/mediawiki.js index 293fd589a4..322c579428 100644 --- a/resources/src/mediawiki/mediawiki.js +++ b/resources/src/mediawiki/mediawiki.js @@ -284,7 +284,7 @@ */ params: function ( parameters ) { var i; - for ( i = 0; i < parameters.length; i += 1 ) { + for ( i = 0; i < parameters.length; i++ ) { this.parameters.push( parameters[ i ] ); } return this; @@ -1039,7 +1039,7 @@ } // Execute all jobs whose dependencies are either all satisfied or contain at least one failed module. - for ( j = 0; j < jobs.length; j += 1 ) { + for ( j = 0; j < jobs.length; j++ ) { hasErrors = anyFailed( jobs[ j ].dependencies ); if ( hasErrors || allReady( jobs[ j ].dependencies ) ) { // All dependencies satisfied, or some have errors @@ -1091,7 +1091,7 @@ * @throws {Error} If any unregistered module or a dependency loop is encountered */ function sortDependencies( module, resolved, unresolved ) { - var n, deps, len, skip; + var i, deps, skip; if ( !hasOwn.call( registry, module ) ) { throw new Error( 'Unknown dependency: ' + module ); @@ -1128,28 +1128,26 @@ } // Tracks down dependencies deps = registry[ module ].dependencies; - len = deps.length; - for ( n = 0; n < len; n += 1 ) { - if ( $.inArray( deps[ n ], resolved ) === -1 ) { - if ( unresolved[ deps[ n ] ] ) { - throw new Error( - 'Circular reference detected: ' + module + - ' -> ' + deps[ n ] - ); + for ( i = 0; i < deps.length; i++ ) { + if ( $.inArray( deps[ i ], resolved ) === -1 ) { + if ( unresolved[ deps[ i ] ] ) { + throw new Error( mw.format( + 'Circular reference detected: $1 -> $2', + module, + deps[ i ] + ) ); } // Add to unresolved unresolved[ module ] = true; - sortDependencies( deps[ n ], resolved, unresolved ); - delete unresolved[ module ]; + sortDependencies( deps[ i ], resolved, unresolved ); } } - resolved[ resolved.length ] = module; + resolved.push( module ); } /** - * Get a list of module names that a module depends on in their proper dependency - * order. + * Get names of module that a module depends on, in their proper dependency order. * * @private * @param {string[]} modules Array of string module names @@ -1164,7 +1162,7 @@ } /** - * Load and execute a script with callback. + * Load and execute a script. * * @private * @param {string} src URL to script, will be used as the src attribute in the script tag @@ -1351,7 +1349,7 @@ // Array of css strings in key 'css', // or back-compat array of urls from media-type if ( $.isArray( value ) ) { - for ( i = 0; i < value.length; i += 1 ) { + for ( i = 0; i < value.length; i++ ) { if ( key === 'bc-url' ) { // back-compat: { : [url, ..] } addLink( media, value[ i ] ); @@ -1366,7 +1364,7 @@ // { "url": { : [url, ..] } } for ( media in value ) { urls = value[ media ]; - for ( i = 0; i < urls.length; i += 1 ) { + for ( i = 0; i < urls.length; i++ ) { addLink( media, urls[ i ] ); } } @@ -1437,7 +1435,7 @@ } } a.sort(); - for ( key = 0; key < a.length; key += 1 ) { + for ( key = 0; key < a.length; key++ ) { sorted[ a[ key ] ] = o[ a[ key ] ]; } return sorted; @@ -1536,12 +1534,12 @@ maxQueryLength = mw.config.get( 'wgResourceLoaderMaxQueryLength', 2000 ); // Appends a list of modules from the queue to the batch - for ( q = 0; q < queue.length; q += 1 ) { + for ( q = 0; q < queue.length; q++ ) { // Only request modules which are registered if ( hasOwn.call( registry, queue[ q ] ) && registry[ queue[ q ] ].state === 'registered' ) { // Prevent duplicate entries if ( $.inArray( queue[ q ], batch ) === -1 ) { - batch[ batch.length ] = queue[ q ]; + batch.push( queue[ q ] ); // Mark registered modules as loading registry[ queue[ q ] ].state = 'loading'; } @@ -1598,7 +1596,7 @@ batch.sort(); // Split batch by source and by group. - for ( b = 0; b < batch.length; b += 1 ) { + for ( b = 0; b < batch.length; b++ ) { bSource = registry[ batch[ b ] ].source; bGroup = registry[ batch[ b ] ].group; if ( !hasOwn.call( splits, bSource ) ) { @@ -1608,7 +1606,7 @@ splits[ bSource ][ bGroup ] = []; } bSourceGroup = splits[ bSource ][ bGroup ]; - bSourceGroup[ bSourceGroup.length ] = batch[ b ]; + bSourceGroup.push( batch[ b ] ); } // Clear the batch - this MUST happen before we append any @@ -1642,7 +1640,7 @@ moduleMap = {}; // { prefix: [ suffixes ] } - for ( i = 0; i < modules.length; i += 1 ) { + for ( i = 0; i < modules.length; i++ ) { // Determine how many bytes this module would add to the query string lastDotIndex = modules[ i ].lastIndexOf( '.' ); @@ -1726,11 +1724,11 @@ * @param {string} [skip=null] Script body of the skip function */ register: function ( module, version, dependencies, group, source, skip ) { - var i, len; + var i; // Allow multiple registration if ( typeof module === 'object' ) { resolveIndexedDependencies( module ); - for ( i = 0, len = module.length; i < len; i++ ) { + for ( i = 0; i < module.length; i++ ) { // module is an array of module names if ( typeof module[ i ] === 'string' ) { mw.loader.register( module[ i ] ); diff --git a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js index d205ba7438..bc25ce09cd 100644 --- a/tests/qunit/suites/resources/mediawiki/mediawiki.test.js +++ b/tests/qunit/suites/resources/mediawiki/mediawiki.test.js @@ -446,7 +446,7 @@ } ); } ); - QUnit.asyncTest( 'mw.loader.using( .. ).promise', 2, function ( assert ) { + QUnit.asyncTest( 'mw.loader.using( .. ) Promise', 2, function ( assert ) { var isAwesomeDone; mw.loader.testCallback = function () { @@ -631,7 +631,7 @@ } ); // @import (bug 31676) - QUnit.asyncTest( 'mw.loader.implement( styles has @import)', 7, function ( assert ) { + QUnit.asyncTest( 'mw.loader.implement( styles has @import )', 7, function ( assert ) { var isJsExecuted, $element; mw.loader.implement( @@ -747,7 +747,7 @@ } ); } ); - QUnit.test( 'mw.loader erroneous indirect dependency', 4, function ( assert ) { + QUnit.test( 'mw.loader with broken indirect dependency', 4, function ( assert ) { // don't emit an error event this.sandbox.stub( mw, 'track' ); @@ -766,6 +766,17 @@ assert.strictEqual( mw.track.callCount, 1 ); } ); + QUnit.test( 'mw.loader with circular dependency', 1, function ( assert ) { + mw.loader.register( [ + [ 'test.circle1', '0', [ 'test.circle2' ] ], + [ 'test.circle2', '0', [ 'test.circle3' ] ], + [ 'test.circle3', '0', [ 'test.circle1' ] ] + ] ); + assert.throws( function () { + mw.loader.using( 'test.circle3' ); + }, /Circular/, 'Detect circular dependency' ); + } ); + QUnit.test( 'mw.loader out-of-order implementation', 9, function ( assert ) { mw.loader.register( [ [ 'test.module4', '0' ], -- 2.20.1