r87986 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r87985‎ | r87986 | r87987 >
Date:15:22, 13 May 2011
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
(reverts r87985, and does what that was SUPPOSED to do!) Gerenalized code used in 2 places for adding scripts to the body, and switched from using jQuery.getScript to direct DOM manipulation to avoid jQuery running script file contents through eval, which caused insane line numbers when errors occured.
Modified paths:
  • /trunk/extensions/ArticleFeedback/modules/ext.articleFeedback/ext.articleFeedback.js (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
@@ -879,8 +879,27 @@
880880 }
881881 return arr.join( '|' ).replace( /\./g, '!' );
882882 }
883 -
884883
 884+ /**
 885+ * Adds a script tag to the body, either using document.write or low-level DOM manipulation,
 886+ * depending on whether document-ready has occured yet.
 887+ */
 888+ function addScript( src ) {
 889+ if ( ready ) {
 890+ // jQuery's getScript method is NOT better than doing this the old-fassioned way
 891+ // because jQuery will eval the script's code, and errors will not have sane
 892+ // line numbers.
 893+ var script = document.createElement( 'script' );
 894+ script.setAttribute( 'src', src );
 895+ script.setAttribute( 'type', 'text/javascript' );
 896+ document.body.appendChild( script );
 897+ } else {
 898+ document.write( mw.html.element(
 899+ 'script', { 'type': 'text/javascript', 'src': src }, ''
 900+ ) );
 901+ }
 902+ }
 903+
885904 /* Public Methods */
886905
887906 /**
@@ -979,23 +998,14 @@
980999 // include modules which are already loaded
9811000 batch = [];
9821001 // Asynchronously append a script tag to the end of the body
983 - var html = '';
9841002 for ( var r = 0; r < requests.length; r++ ) {
9851003 requests[r] = sortQuery( requests[r] );
986 - // Build out the HTML
9871004 var src = mw.config.get( 'wgLoadScript' ) + '?' + $.param( requests[r] );
988 - html += mw.html.element( 'script',
989 - { 'type': 'text/javascript', 'src': src }, '' );
 1005+ addScript( src );
9901006 }
991 - // Load asynchronously after documument ready
992 - if ( ready ) {
993 - setTimeout( function() { $( 'body' ).append( html ); }, 0 );
994 - } else {
995 - document.write( html );
996 - }
9971007 }
9981008 };
999 -
 1009+
10001010 /**
10011011 * Registers a module, letting the system know about it and its
10021012 * dependencies. loader.js files contain calls to this function.
@@ -1156,13 +1166,7 @@
11571167 } ) );
11581168 return true;
11591169 } else if ( type === 'text/javascript' || typeof type === 'undefined' ) {
1160 - var script = mw.html.element( 'script',
1161 - { type: 'text/javascript', src: modules }, '' );
1162 - if ( ready ) {
1163 - $( 'body' ).append( script );
1164 - } else {
1165 - document.write( script );
1166 - }
 1170+ addScript( modules );
11671171 return true;
11681172 }
11691173 // Unknown type
@@ -1322,7 +1326,6 @@
13231327 };
13241328 } )();
13251329
1326 -
13271330 /* Extension points */
13281331
13291332 this.legacy = {};
Index: trunk/extensions/ArticleFeedback/modules/ext.articleFeedback/ext.articleFeedback.js
@@ -29,8 +29,6 @@
3030 return $.cookie( prefix( 'pitches-' + pitch ) ) != 'hide';
3131 }
3232
33 -dknvlsdnvlsknvlsekvnseoivleskmvleskmvlesjk + 1 = sdsd();
34 -
3533 /**
3634 * Ensures a pitch will be muted for a given duration of time
3735 *

Follow-up revisions

RevisionCommit summaryAuthorDate
r96679MFT r87986 -- consolidation of addScript in mediawiki.loader, also fixes bug ...brion18:39, 9 September 2011
r96680Followup r87986: qunit test case for bug 30825...brion18:43, 9 September 2011

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r87985Gerenalized code used in 2 places for adding scripts to the body, and switche...tparscal15:21, 13 May 2011

Comments

#Comment by Brion VIBBER (talk | contribs)   18:20, 9 September 2011

As a happy side effect, using the raw DOM append here (thus skipping some weird hidden behavior of jQuery where its .append() etc actually end up loading scripts through $.evalScript) this also fixes bug 30825. Needs backport to 1.17wmf1.

Status & tagging log