r75170 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r75169‎ | r75170 | r75171 >
Date:21:25, 21 October 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
ResourceLoaderFileModule now uses mediaWiki.loader.load to load scripts in debug mode.
Modified paths:
  • /trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/resourceloader/ResourceLoaderFileModule.php
@@ -166,7 +166,7 @@
167167 * @return {string} JavaScript code for $context
168168 */
169169 public function getScript( ResourceLoaderContext $context ) {
170 - global $wgScriptPath;
 170+ global $wgServer, $wgScriptPath;
171171
172172 $files = array_merge(
173173 $this->scripts,
@@ -176,11 +176,12 @@
177177 if ( $context->getDebug() ) {
178178 $files = array_merge( $files, $this->debugScripts );
179179 if ( $this->debugRaw ) {
180 - $tags = '';
 180+ $script = '';
181181 foreach ( $files as $file ) {
182 - $tags .= "<script type=\"text/javascript\" src=\"$wgScriptPath/$file\"></script>";
 182+ $path = FormatJson::encode( "$wgServer$wgScriptPath/$file" );
 183+ $script .= "\n\tmediaWiki.loader.load( $path );";
183184 }
184 - return "\n\tdocument.write( " . FormatJson::encode( $tags ) . ' );';
 185+ return $script;
185186 }
186187 }
187188 return self::readScriptFiles( $files );
Index: trunk/phase3/resources/mediawiki/mediawiki.js
@@ -673,14 +673,15 @@
674674 // Support adding arbitrary external scripts
675675 if ( modules.substr( 0, 7 ) == 'http://' || modules.substr( 0, 8 ) == 'https://' ) {
676676 if ( type === 'text/css' ) {
677 - setTimeout( function() {
678 - $( 'head' ).append( '<link rel="stylesheet" type="text/css" />' ).attr( 'href', modules );
679 - }, 0 );
 677+ $( 'head' ).append( $( '<link rel="stylesheet" type="text/css" />' ).attr( 'href', modules ) );
680678 return true;
681679 } else if ( type === 'text/javascript' || typeof type === 'undefined' ) {
682 - setTimeout( function() {
683 - $( 'body' ).append( '<script type="text/javascript"></script>' ).attr( 'src', modules )
684 - }, 0 );
 680+ var script = '<script type="text/javascript" src="' + modules + '"></script>';
 681+ if ( ready ) {
 682+ $( 'body' ).append( script );
 683+ } else {
 684+ document.write( script );
 685+ }
685686 return true;
686687 }
687688 // Unknown type

Follow-up revisions

RevisionCommit summaryAuthorDate
r75404Merging from trunk r75170:75401awjrichards23:57, 25 October 2010
r76283* Moved htmlEscape from mediawiki.util.js to mediawiki.js so that it can be u...tstarling23:46, 7 November 2010

Comments

#Comment by Catrope (talk | contribs)   11:41, 27 October 2010
-							$( 'body' ).append( '<script type="text/javascript"></script>'  ).attr( 'src', modules )
-						}, 0 );
+						var script = '<script type="text/javascript" src="' + modules + '"></script>';

This removes the escaping of modules.

#Comment by Trevor Parscal (WMF) (talk | contribs)   16:51, 27 October 2010

Well the old one didn't work either, it just appened a script tag and set the src attribute to the body. Also, the reason the script tag is built this way is because when I used jQuery it caused the script to be loaded after document ready.

#Comment by Catrope (talk | contribs)   16:53, 27 October 2010

What if you use jQuery to build the tag but document.write to write it? That should work, right?

#Comment by Trevor Parscal (WMF) (talk | contribs)   17:17, 27 October 2010

No, it doesn't.

#Comment by Catrope (talk | contribs)   19:37, 1 November 2010

I played around with this a bit myself and found that jQuery is very unwilling to construct a script tag, then produce the HTML for it without trying to load the remote script first.

So not escaping the module list is far from ideal but I don't see any other workable solution.

#Comment by Trevor Parscal (WMF) (talk | contribs)   20:39, 1 November 2010

Yes, exactly. I'm sure there's a way to escape the data though.

#Comment by Krinkle (talk | contribs)   16:30, 8 November 2010

Most if not all browser escape attribute values automatically when assigned directly. ie. the following in the console

> a = new Image()
> a.src = '[http://test.com/?var= http://test.com/?var=]<test>'
> a.src
"[http://test.com/?var=%3Ctest%3E http://test.com/?var=%3Ctest%3E]"
#Comment by Krinkle (talk | contribs)   16:32, 8 November 2010

grmpf, CR and/or MediaWiki is screwing that up

> a = new Image()
> a.src = '[http://test.com/?var= http://test.com/?var=]<test>'
> a.src
"[http://test.com/?var=%3Ctest%3E http://test.com/?var=%3Ctest%3E]"
#Comment by Krinkle (talk | contribs)   16:32, 8 November 2010

Okay, just pretend the link isn't repeated twice and with the [ brackets.

Status & tagging log