r70483 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r70482‎ | r70483 | r70484 >
Date:22:02, 4 August 2010
Author:tparscal
Status:resolved (Comments)
Tags:
Comment:
Started integrating ResourceLoader into OutputPage. Fixed a bug that made the mediaWiki object the window rather than window.mediaWiki. Also renamed MediaWiki to mediaWiki, and am not exporting mediaWiki as mw...
Modified paths:
  • /branches/resourceloader/phase3/includes/OutputPage.php (modified) (history)
  • /branches/resourceloader/phase3/includes/ResourceLoader.php (modified) (history)
  • /branches/resourceloader/phase3/includes/Skin.php (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.IEFixes.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.ajaxwatch.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.block.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.changepassword.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.edit.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.enhancedchanges.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.history.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.htmlform.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.prefs.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.preview.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.protect.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.rightclickedit.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.search.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.upload.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/mediawiki.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/mediawiki.log.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/views/mediawiki.views.diff.js (modified) (history)
  • /branches/resourceloader/phase3/resources/mediawiki/views/mediawiki.views.install.js (modified) (history)

Diff [purge]

Index: branches/resourceloader/phase3/includes/OutputPage.php
——@@ -2176,7 +2176,18 @@
21772177
21782178 return $ret;
21792179 }
2180 -
 2180+
 2181+ static function makeResourceLoaderLinkedScript( $skin, $modules ) {
 2182+ global $wgUser, $wgLang, $wgRequest, $wgScriptPath;
 2183+ $query = array(
 2184+ 'modules' => implode( '|', $modules ),
 2185+ 'user' => $wgUser->isLoggedIn(),
 2186+ 'lang' => $wgLang->getCode(),
 2187+ 'debug' => ( $wgRequest->getVal( 'debug' ) === 'true' ),
 2188+ );
 2189+ return Html::linkedScript( "{$wgScriptPath}/load.php?" . http_build_query( $query ) );
 2190+ }
 2191+
21812192 /**
21822193 * Gets the global variables and mScripts; also adds userjs to the end if
21832194 * enabled
——@@ -2187,10 +2198,14 @@
21882199 function getHeadScripts( Skin $sk ) {
21892200 global $wgUser, $wgRequest, $wgJsMimeType, $wgUseSiteJs;
21902201 global $wgStylePath, $wgStyleVersion;
2191 -
2192 - $scripts = Skin::makeGlobalVariablesScript( $sk->getSkinName() ) . "\n";
2193 - $scripts .= Html::linkedScript( "{$wgStylePath}/common/wikibits.js?$wgStyleVersion" );
2194 -
 2202+
 2203+ // Include base modules
 2204+ $scripts = self::makeResourceLoaderLinkedScript( $sk, array( 'jquery', 'mediawiki' ) );
 2205+ // Configure page
 2206+ $scripts .= Skin::makeGlobalVariablesScript( $sk->getSkinName() ) . "\n";
 2207+ // Wikibits legacy code
 2208+ $scripts .= self::makeResourceLoaderLinkedScript( $sk, array( 'mediawiki.legacy.wikibits' ) );
 2209+
21952210 // add site JS if enabled
21962211 if( $wgUseSiteJs ) {
21972212 $jsCache = $wgUser->isLoggedIn() ? '&smaxage=0' : '';
Index: branches/resourceloader/phase3/includes/ResourceLoader.php
——@@ -82,19 +82,23 @@
8383 if ( $cached !== false && $cached !== null ) {
8484 return $cached;
8585 }
86 - switch ( $filter ) {
87 - case 'minify-js':
88 - $result = JSMin::minify( $data );
89 - break;
90 - case 'minify-css':
91 - $result = Minify_CSS::minify( $data, array( 'currentDir' => dirname( $file ), 'docRoot' => '.' ) );
92 - break;
93 - case 'flip-css':
94 - $result = CSSJanus::transform( $data, true, false );
95 - break;
96 - default:
97 - // Don't cache anything, just pass right through
98 - return $data;
 86+ try {
 87+ switch ( $filter ) {
 88+ case 'minify-js':
 89+ $result = JSMin::minify( $data );
 90+ break;
 91+ case 'minify-css':
 92+ $result = Minify_CSS::minify( $data, array( 'currentDir' => dirname( $file ), 'docRoot' => '.' ) );
 93+ break;
 94+ case 'flip-css':
 95+ $result = CSSJanus::transform( $data, true, false );
 96+ break;
 97+ default:
 98+ // Don't cache anything, just pass right through
 99+ return $data;
 100+ }
 101+ } catch ( Exception $exception ) {
 102+ throw new MWException( 'Filter threw an exception: ' . $exception->getMessage() );
99103 }
100104 $wgMemc->set( $key, $result );
101105 return $result;
——@@ -272,8 +276,8 @@
273277 );
274278 // Mediawiki's WebRequest::getBool is a bit on the annoying side - we need to allow 'true' and 'false' values
275279 // to be converted to boolean true and false
276 - $parameters['user'] = $parameters['user'] === 'true' || $parameters['user'] === true;
277 - $parameters['debug'] = $parameters['debug'] === 'true' || $parameters['debug'] === true;
 280+ $parameters['user'] = $parameters['user'] === 'true';
 281+ $parameters['debug'] = $parameters['debug'] === 'true';
278282 // Get the direction from the requested language
279283 if ( !isset( $parameters['dir'] ) ) {
280284 $lang = $wgLang->factory( $parameters['lang'] );
——@@ -305,16 +309,7 @@
306310 }
307311 // Special meta-information for the 'mediawiki' module
308312 if ( in_array( 'mediawiki', $modules ) ) {
309 - /*
310 - * Skin::makeGlobalVariablesScript needs to be modified so that we still output the globals for now, but
311 - * also put them into the initial payload like this:
312 - *
313 - * // Sets the inital configuration
314 - * mw.config.set( { 'name': 'value', ... } );
315 - *
316 - * Also, the naming of these variables is horrible and sad, hopefully this can be worked on
317 - */
318 - echo "mw.config.set( " . json_encode( $parameters ) . " );\n";
 313+ echo "mediaWiki.config.set( 'debug', " . ( $parameters['debug'] ? 'true' : 'false' ) . " );\n";
319314 // Generate list of registrations and collect all loader scripts
320315 $loaders = array();
321316 $registrations = array();
——@@ -337,7 +332,7 @@
338333 // Include loaders
339334 self::read( $loaders, true );
340335 // Register modules without loaders
341 - echo "mw.loader.register( " . json_encode( array_values( $registrations ) ) . " );\n";
 336+ echo "mediaWiki.loader.register( " . json_encode( array_values( $registrations ) ) . " );\n";
342337 }
343338 // Output non-raw modules
344339 $blobs = MessageBlobStore::get( $modules, $parameters['lang'] );
——@@ -373,7 +368,7 @@
374369 // Messages
375370 $messages = isset( $blobs[$module] ) ? $blobs[$module] : '{}';
376371 // Output
377 - echo "mw.loader.implement( '{$module}', function() {\n{$script}\n}, '{$style}', {$messages} );\n";
 372+ echo "mediaWiki.loader.implement( '{$module}', function() {\n{$script}\n}, '{$style}', {$messages} );\n";
378373 }
379374 }
380375 // Set headers -- when we support CSS only mode, this might change!
Index: branches/resourceloader/phase3/includes/Skin.php
——@@ -339,16 +339,10 @@
340340 $out->out( "\n</body></html>" );
341341 wfProfileOut( __METHOD__ );
342342 }
343 -
 343+
344344 static function makeVariablesScript( $data ) {
345345 if( $data ) {
346 - $r = array();
347 - foreach ( $data as $name => $value ) {
348 - $encValue = Xml::encodeJsVar( $value );
349 - $r[] = "$name=$encValue";
350 - }
351 - $js = 'var ' . implode( ",\n", $r ) . ';';
352 - return Html::inlineScript( "\n$js\n" );
 346+ return Html::inlineScript( 'mediaWiki.config.set(' . json_encode( $data ) . ');' );
353347 } else {
354348 return '';
355349 }
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.history.js
——@@ -112,4 +112,4 @@
113113 mw.legacy.histrowinit();
114114 } );
115115
116 -} )( jQuery, MediaWiki );
\ No newline at end of file
 116+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.search.js
——@@ -60,4 +60,4 @@
6161 }
6262 } );
6363
64 -} )( jQuery, MediaWiki );
\ No newline at end of file
 64+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.changepassword.js
——@@ -26,4 +26,4 @@
2727 mw.legacy.onNameChangeHook();
2828 } );
2929
30 -} )( jQuery, MediaWiki );
\ No newline at end of file
 30+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.ajaxwatch.js
——@@ -133,4 +133,4 @@
134134 mw.legacy.wgAjaxWatch.$links = $links;
135135 } );
136136
137 -} )( jQuery, MediaWiki );
\ No newline at end of file
 137+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.prefs.js
——@@ -230,4 +230,4 @@
231231 mw.legacy.tabbedprefs();
232232 } );
233233
234 -} )( jQuery, MediaWiki );
\ No newline at end of file
 234+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.protect.js
——@@ -343,4 +343,4 @@
344344 }
345345 } );
346346
347 -} )( jQuery, MediaWiki );
\ No newline at end of file
 347+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.preview.js
——@@ -118,4 +118,4 @@
119119 $j('#wpPreview').click( doLivePreview );
120120 } );
121121
122 -} )( jQuery, MediaWiki );
\ No newline at end of file
 122+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.upload.js
——@@ -330,4 +330,4 @@
331331 mw.legacy.wgUploadSetup();
332332 } );
333333
334 -} )( jQuery, MediaWiki );
\ No newline at end of file
 334+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.edit.js
——@@ -261,4 +261,4 @@
262262 } );
263263 } );
264264
265 -} )( jQuery, MediaWiki );
\ No newline at end of file
 265+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.htmlform.js
——@@ -47,4 +47,4 @@
4848 } );
4949 } );
5050
51 -} )( jQuery, MediaWiki );
\ No newline at end of file
 51+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.rightclickedit.js
——@@ -62,4 +62,4 @@
6363 mw.legacy.setupRightClickEdit();
6464 } );
6565
66 -} )( jQuery, MediaWiki );
\ No newline at end of file
 66+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.block.js
——@@ -41,4 +41,4 @@
4242 mw.legacy.considerChangingExpiryFocus();
4343 } );
4444
45 -} )( jQuery, MediaWiki );
\ No newline at end of file
 45+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.IEFixes.js
——@@ -138,4 +138,4 @@
139139 mw.legacy.hookit();
140140 } );
141141
142 -} )( jQuery, MediaWiki );
\ No newline at end of file
 142+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/legacy/mediawiki.legacy.enhancedchanges.js
——@@ -43,4 +43,4 @@
4444 );
4545 } );
4646
47 -} )( jQuery, MediaWiki );
\ No newline at end of file
 47+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/views/mediawiki.views.install.js
——@@ -97,4 +97,4 @@
9898 } );
9999 } );
100100
101 -} )( jQuery, MediaWiki );
\ No newline at end of file
 101+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/views/mediawiki.views.diff.js
——@@ -27,4 +27,4 @@
2828 }
2929 } );
3030
31 -} )( jQuery, MediaWiki );
\ No newline at end of file
 31+} )( jQuery, mediaWiki );
\ No newline at end of file
Index: branches/resourceloader/phase3/resources/mediawiki/mediawiki.js
——@@ -36,7 +36,8 @@
3737 /*
3838 * Core MediaWiki JavaScript Library
3939 */
40 -( function( $ ) {
 40+// Attach to window
 41+window.mediaWiki = new ( function( $ ) {
4142
4243 /* Constants */
4344
——@@ -90,10 +91,14 @@
9192 }
9293 }
9394 return result;
94 - } else if ( typeof values[keys] === 'undefined' ) {
95 - return typeof fallback !== 'undefined' ? fallback : null;
 95+ } else if ( typeof keys === 'string' ) {
 96+ if ( typeof values[keys] === 'undefined' ) {
 97+ return typeof fallback !== 'undefined' ? fallback : null;
 98+ } else {
 99+ return values[keys];
 100+ }
96101 } else {
97 - return values[keys];
 102+ return values;
98103 }
99104 };
100105 /**
——@@ -285,7 +290,7 @@
286291 }
287292 // Add localizations to message system
288293 if ( typeof registry[module].messages === 'object' ) {
289 - mw.msg.set( registry[module].messages );
 294+ mediaWiki.msg.set( registry[module].messages );
290295 }
291296 // Execute script
292297 try {
——@@ -310,7 +315,7 @@
311316 }
312317 }
313318 } catch ( e ) {
314 - mw.log( 'Exception thrown by ' + module + ': ' + e.message );
 319+ mediaWiki.log( 'Exception thrown by ' + module + ': ' + e.message );
315320 registry[module].state = 'error';
316321 // Run error callbacks of jobs affected by this condition
317322 for ( var j = 0; j < jobs.length; j++ ) {
——@@ -387,7 +392,12 @@
388393 // Always order modules alphabetically to help reduce cache misses for otherwise identical content
389394 batch.sort();
390395 // Build a list of request parameters
391 - var base = mw.config.get( [ 'user', 'skin', 'lang', 'debug' ] );
 396+ var base = {
 397+ 'user': mediaWiki.config.get( 'wgUserName' ) !== null,
 398+ 'skin': mediaWiki.config.get( 'skin' ),
 399+ 'lang': mediaWiki.config.get( 'wgUserLanguage' ),
 400+ 'skin': mediaWiki.config.get( 'debug' ),
 401+ };
392402 // Extend request parameters with a list of modules in the batch
393403 var requests = [];
394404 if ( base.debug == '1' ) {
——@@ -406,7 +416,7 @@
407417 var html = '';
408418 for ( var r = 0; r < requests.length; r++ ) {
409419 // Build out the HTML
410 - var src = mw.config.get( 'server' ) + '/load.php?' + jQuery.param( requests[r] );
 420+ var src = mediaWiki.config.get( 'server' ) + '/load.php?' + jQuery.param( requests[r] );
411421 html += '<script type="text/javascript" src="' + src + '"></script>';
412422 }
413423 // Append script to body
——@@ -535,7 +545,7 @@
536546 }
537547 // Allow calling with a single need as a string
538548 if ( typeof modules === 'string' ) {
539 - modules = [needs];
 549+ modules = [modules];
540550 }
541551 // Resolve entire dependency map
542552 modules = resolve( modules );
——@@ -565,7 +575,4 @@
566576 /* Extension points */
567577
568578 this.utilities = {};
569 -
570 - // Attach to window
571 - window.MediaWiki = window.mw = this;
572 -} )( jQuery );
\ No newline at end of file
 579+} )( jQuery );
Index: branches/resourceloader/phase3/resources/mediawiki/mediawiki.log.js
——@@ -60,4 +60,4 @@
6161 }
6262 } );
6363
64 -} )( jQuery, MediaWiki );
\ No newline at end of file
 64+} )( jQuery, mediaWiki );
\ No newline at end of file

Follow-up revisions

RevisionCommit summaryAuthorDate
r70579Using $ instead of jQuery - resolved nudge about r70483tparscal18:40, 6 August 2010

Comments

#Comment by Catrope (talk | contribs)   11:53, 6 August 2010 
+			'debug' => ( $wgRequest->getVal( 'debug' ) === 'true' ),

Why not just pass the value of debug on verbatim and let the resource loader parse it? 

+		return Html::linkedScript( "{$wgScriptPath}/load.php?" . http_build_query( $query ) );

We have wfAppendQuery( "$wgScriptPath/load.php", $query ) for this. 

+		$scripts = self::makeResourceLoaderLinkedScript( $sk, array( 'jquery', 'mediawiki' ) );
+		// Configure page
+		$scripts .= Skin::makeGlobalVariablesScript( $sk->getSkinName() ) . "\n";
+		// Wikibits legacy code
+		$scripts .= self::makeResourceLoaderLinkedScript( $sk, array( 'mediawiki.legacy.wikibits' ) );

Would it be possible to change the order so the two makeResourceLoaderLinkedScript() calls can be merged? 

-		$parameters['user'] = $parameters['user'] === 'true' || $parameters['user'] === true;
-		$parameters['debug'] = $parameters['debug'] === 'true' || $parameters['debug'] === true;
+		$parameters['user'] = $parameters['user'] === 'true';
+		$parameters['debug'] = $parameters['debug'] === 'true';

This means only the exact value 'true' triggers debug/user, and anything else, even 1, is read as false. I don't think this is desirable.

This commit uses json_encode() in a few places; these uses should be replaced with FormatJson::encode(). 

+				var base = {
+					'skin': mediaWiki.config.get( 'skin' ),
[...]
+					'skin': mediaWiki.config.get( 'debug' ),

Second line should read 'debug': ... 

						var src = mediaWiki.config.get( 'server' ) + '/load.php?' + jQuery.param( requests[r] );

Why not just use $ like everywhere else?


#Comment by Trevor Parscal (WMF) (talk | contribs)   18:57, 6 August 2010

1. If we don't normalize the 'true' or 'false' values on the server, we end up with inconsistent versions of it floating around... I think. To be honest, we should talk more about this kind of thing. 2. Fixed in r70582 3. Probably, this is a mess right now - hope to get it cleaned up and optimized soon. 4. Dang it - you know what, I hate this 'true' 'false' 1 0 thing! 5. Oops... fixed in r70579

#Comment by Catrope (talk | contribs)   13:54, 7 August 2010
  1. Exactly, so we should normalize them in one place (ResourceLoader), not in two (ResourceLoader and OutputPage)
  2. Looks good
  3. OK, take your time
  4. Yes, it's a nightmare
  5. Looks good

Status & tagging log