r110592 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r110591‎ | r110592 | r110593 >
Date:16:38, 2 February 2012
Author:catrope
Status:ok (Comments)
Tags:
Comment:
Followup r108184: per CR, blocking loads in debug mode were broken because debug mode uses chained loading, and the second part of the chain would run after the blocking flag had been set back to false. So get rid of the blocking flag as a global state flag, and instead pass it around all over the place so nestedAddScript() can use it.
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
——@@ -2695,9 +2695,7 @@
26962696 if ( $modules ) {
26972697 $scripts .= Html::inlineScript(
26982698 ResourceLoader::makeLoaderConditionalScript(
2699 - "mw.loader.setBlocking( true );\n" .
2700 - Xml::encodeJsCall( 'mw.loader.load', array( $modules ) ) .
2701 - "\nmw.loader.setBlocking( false );"
 2699+ Xml::encodeJsCall( 'mw.loader.load', array( $modules, null, true ) )
27022700 )
27032701 );
27042702 }
Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -364,9 +364,6 @@
365365 jobs = [],
366366 // Flag indicating that document ready has occured
367367 ready = false,
368 - // Whether we should try to load scripts in a blocking way
369 - // Set with setBlocking()
370 - blocking = false,
371368 // Selector cache for the marker element. Use getMarker() to get/use the marker!
372369 $marker = null;
373370
——@@ -549,7 +546,7 @@
550547 var j, r;
551548
552549 try {
553 - // Run jobs who's dependencies have just been met
 550+ // Run jobs whose dependencies have just been met
554551 for ( j = 0; j < jobs.length; j += 1 ) {
555552 if ( compare(
556553 filter( 'ready', jobs[j].dependencies ),
——@@ -562,7 +559,7 @@
563560 j -= 1;
564561 }
565562 }
566 - // Execute modules who's dependencies have just been met
 563+ // Execute modules whose dependencies have just been met
567564 for ( r in registry ) {
568565 if ( registry[r].state === 'loaded' ) {
569566 if ( compare(
——@@ -594,7 +591,7 @@
595592 * @param src String: URL to script, will be used as the src attribute in the script tag
596593 * @param callback Function: Optional callback which will be run when the script is done
597594 */
598 - function addScript( src, callback ) {
 595+ function addScript( src, callback, blocking ) {
599596 var done = false, script, head;
600597 if ( ready || !blocking ) {
601598 // jQuery's getScript method is NOT better than doing this the old-fashioned way
——@@ -710,7 +707,7 @@
711708 callback();
712709 }
713710 };
714 - nestedAddScript = function ( arr, callback, i ) {
 711+ nestedAddScript = function ( arr, callback, blocking, i ) {
715712 // Recursively call addScript() in its own callback
716713 // for each element of arr.
717714 if ( i >= arr.length ) {
——@@ -720,13 +717,13 @@
721718 }
722719
723720 addScript( arr[i], function() {
724 - nestedAddScript( arr, callback, i + 1 );
725 - } );
 721+ nestedAddScript( arr, callback, blocking, i + 1 );
 722+ }, blocking );
726723 };
727724
728725 if ( $.isArray( script ) ) {
729726 registry[module].state = 'loading';
730 - nestedAddScript( script, markModuleReady, 0 );
 727+ nestedAddScript( script, markModuleReady, registry[module].blocking, 0 );
731728 } else if ( $.isFunction( script ) ) {
732729 script( $ );
733730 markModuleReady();
——@@ -749,8 +746,10 @@
750747 * @param dependencies string module name or array of string module names
751748 * @param ready function callback to execute when all dependencies are ready
752749 * @param error function callback to execute when any dependency fails
 750+ * @param blocking (optional) If true, load modules in a blocking fashion if
 751+ * document ready has not yet occurred
753752 */
754 - function request( dependencies, ready, error ) {
 753+ function request( dependencies, ready, error, blocking ) {
755754 var regItemDeps, regItemDepLen, n;
756755
757756 // Allow calling by single module name
——@@ -782,6 +781,10 @@
783782 for ( n = 0; n < dependencies.length; n += 1 ) {
784783 if ( $.inArray( dependencies[n], queue ) === -1 ) {
785784 queue[queue.length] = dependencies[n];
 785+ if ( blocking ) {
 786+ // Mark this module as blocking in the registry
 787+ registry[dependencies[n]].blocking = true;
 788+ }
786789 }
787790 }
788791 // Work the queue
——@@ -821,8 +824,9 @@
822825 * @param moduleMap {Object}: Module map, see buildModulesString()
823826 * @param currReqBase {Object}: Object with other parameters (other than 'modules') to use in the request
824827 * @param sourceLoadScript {String}: URL of load.php
 828+ * @param blocking {Boolean}: If true, use a blocking request if document ready has not yet occurred
825829 */
826 - function doRequest( moduleMap, currReqBase, sourceLoadScript ) {
 830+ function doRequest( moduleMap, currReqBase, sourceLoadScript, blocking ) {
827831 var request = $.extend(
828832 { 'modules': buildModulesString( moduleMap ) },
829833 currReqBase
——@@ -830,7 +834,7 @@
831835 request = sortQuery( request );
832836 // Asynchronously append a script tag to the end of the body
833837 // Append &* to avoid triggering the IE6 extension check
834 - addScript( sourceLoadScript + '?' + $.param( request ) + '&*' );
 838+ addScript( sourceLoadScript + '?' + $.param( request ) + '&*', null, blocking );
835839 }
836840
837841 /* Public Methods */
——@@ -842,7 +846,7 @@
843847 var reqBase, splits, maxQueryLength, q, b, bSource, bGroup, bSourceGroup,
844848 source, group, g, i, modules, maxVersion, sourceLoadScript,
845849 currReqBase, currReqBaseLength, moduleMap, l,
846 - lastDotIndex, prefix, suffix, bytesAdded;
 850+ lastDotIndex, prefix, suffix, bytesAdded, blocking;
847851
848852 // Build a list of request parameters common to all requests.
849853 reqBase = {
——@@ -919,7 +923,7 @@
920924
921925 currReqBase = $.extend( { 'version': formatVersionNumber( maxVersion ) }, reqBase );
922926 currReqBaseLength = $.param( currReqBase ).length;
923 - moduleMap = {};
 927+ blocking = false;
924928 // We may need to split up the request to honor the query string length limit,
925929 // so build it piece by piece.
926930 l = currReqBaseLength + 9; // '&modules='.length == 9
——@@ -941,19 +945,26 @@
942946 if ( maxQueryLength > 0 && !$.isEmptyObject( moduleMap ) && l + bytesAdded > maxQueryLength ) {
943947 // This request would become too long, create a new one
944948 // and fire off the old one
945 - doRequest( moduleMap, currReqBase, sourceLoadScript );
 949+ doRequest( moduleMap, currReqBase, sourceLoadScript, blocking );
946950 moduleMap = {};
 951+ blocking = false;
947952 l = currReqBaseLength + 9;
948953 }
949954 if ( moduleMap[prefix] === undefined ) {
950955 moduleMap[prefix] = [];
951956 }
952957 moduleMap[prefix].push( suffix );
 958+ if ( registry[modules[i]].blocking ) {
 959+ // If this module is blocking, make the entire request blocking
 960+ // This is slightly suboptimal, but in practice mixing of blocking
 961+ // and non-blocking modules will only occur in debug mode.
 962+ blocking = true;
 963+ }
953964 l += bytesAdded;
954965 }
955966 // If there's anything left in moduleMap, request that too
956967 if ( !$.isEmptyObject( moduleMap ) ) {
957 - doRequest( moduleMap, currReqBase, sourceLoadScript );
 968+ doRequest( moduleMap, currReqBase, sourceLoadScript, blocking );
958969 }
959970 }
960971 }
——@@ -1135,8 +1146,10 @@
11361147 * @param type {String} mime-type to use if calling with a URL of an
11371148 * external script or style; acceptable values are "text/css" and
11381149 * "text/javascript"; if no type is provided, text/javascript is assumed.
 1150+ * @param blocking {Boolean} (optional) If true, load modules in a blocking
 1151+ * fashion if document ready has not yet occurred
11391152 */
1140 - load: function ( modules, type ) {
 1153+ load: function ( modules, type, blocking ) {
11411154 var filtered, m;
11421155
11431156 // Validate input
——@@ -1155,7 +1168,7 @@
11561169 } ) );
11571170 return;
11581171 } else if ( type === 'text/javascript' || type === undefined ) {
1159 - addScript( modules );
 1172+ addScript( modules, null, blocking );
11601173 return;
11611174 }
11621175 // Unknown type
——@@ -1188,7 +1201,7 @@
11891202 }
11901203 // Since some modules are not yet ready, queue up a request
11911204 else {
1192 - request( filtered );
 1205+ request( filtered, null, null, blocking );
11931206 return;
11941207 }
11951208 },
——@@ -1254,18 +1267,6 @@
12551268 return key;
12561269 } );
12571270 },
1258 -
1259 - /**
1260 - * Enable or disable blocking. If blocking is enabled and
1261 - * document ready has not yet occurred, scripts will be loaded
1262 - * in a blocking way (using document.write) rather than
1263 - * asynchronously using DOM manipulation
1264 - *
1265 - * @param b {Boolean} True to enable blocking, false to disable it
1266 - */
1267 - setBlocking: function( b ) {
1268 - blocking = b;
1269 - },
12701271
12711272 /**
12721273 * For backwards-compatibility with Squid-cached pages. Loads mw.user

Follow-up revisions

RevisionCommit summaryAuthorDate
r111597Followup r110592: rename 'blocking' to 'async', and invert the logic everywhe...catrope23:38, 15 February 2012

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r108184ResourceLoader: Add an experimental option to move the main module loading qu...catrope23:32, 5 January 2012

Comments

#Comment by Aaron Schulz (talk | contribs)   17:40, 2 February 2012

"pass it around all over the place"...sounds evil :)

#Comment by Catrope (talk | contribs)   16:15, 8 February 2012

Yeah, it's not the cleanest code I ever wrote, but it's needed because the asynchronous nature of things means that an object-wide state variable doesn't cut it.

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:45, 14 February 2012

Seems like a justified evil. Could probably be refactored somehow, but atm, this looks fine to me.

Status & tagging log