r73673 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r73672‎ | r73673 | r73674 >
Date:18:49, 24 September 2010
Author:tparscal
Status:resolved (Comments)
Tags:
Comment:
* Moved registration generation to startup module.
* Moved some javascript code generation to reusable functions (more to do).
* Reduced the code output by using mutliple calling method for mediaWiki.loader.state.
* Moved CSS minification to the end (should be a bit faster than running it for each module).
Modified paths:
  • /trunk/phase3/includes/ResourceLoader.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoaderContext.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoaderModule.php (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/ResourceLoaderContext.php
——@@ -27,6 +27,7 @@
2828 * of a specific loader request
2929 */
3030 class ResourceLoaderContext {
 31+
3132 /* Protected Members */
3233
3334 protected $request;
Index: trunk/phase3/includes/ResourceLoader.php
——@@ -53,7 +53,16 @@
5454 }
5555 }
5656
57 - protected static function preloadModuleInfo( $modules, ResourceLoaderContext $context ) {
 57+ /*
 58+ * Loads information stored in the database about modules
 59+ *
 60+ * This is not inside the module code because it's so much more performant to request all of the information at once
 61+ * than it is to have each module requests it's own information.
 62+ *
 63+ * @param $modules array list of modules to preload information for
 64+ * @param $context ResourceLoaderContext context to load the information within
 65+ */
 66+ protected static function preloadModuleInfo( array $modules, ResourceLoaderContext $context ) {
5867 $dbr = wfGetDb( DB_SLAVE );
5968 $skin = $context->getSkin();
6069 $lang = $context->getLanguage();
——@@ -107,8 +116,7 @@
108117 *
109118 * @param $filter String: name of filter to run
110119 * @param $data String: text to filter, such as JavaScript or CSS text
111 - * @param $file String: path to file being filtered, (optional: only required
112 - * for CSS to resolve paths)
 120+ * @param $file String: path to file being filtered, (optional: only required for CSS to resolve paths)
113121 * @return String: filtered data
114122 */
115123 protected static function filter( $filter, $data ) {
——@@ -227,54 +235,6 @@
228236 }
229237
230238 /**
231 - * Gets registration code for all modules
232 - *
233 - * @param $context ResourceLoaderContext object
234 - * @return String: JavaScript code for registering all modules with the client loader
235 - */
236 - public static function getModuleRegistrations( ResourceLoaderContext $context ) {
237 - wfProfileIn( __METHOD__ );
238 - self::initialize();
239 -
240 - $scripts = '';
241 - $registrations = array();
242 -
243 - foreach ( self::$modules as $name => $module ) {
244 - // Support module loader scripts
245 - if ( ( $loader = $module->getLoaderScript() ) !== false ) {
246 - $deps = FormatJson::encode( $module->getDependencies() );
247 - $group = FormatJson::encode( $module->getGroup() );
248 - $version = wfTimestamp( TS_ISO_8601, round( $module->getModifiedTime( $context ), -2 ) );
249 - $scripts .= "( function( name, version, dependencies ) { $loader } )\n" .
250 - "( '$name', '$version', $deps, $group );\n";
251 - }
252 - // Automatically register module
253 - else {
254 - // Modules without dependencies or a group pass two arguments (name, timestamp) to
255 - // mediaWiki.loader.register()
256 - if ( !count( $module->getDependencies() && $module->getGroup() === null ) ) {
257 - $registrations[] = array( $name, $module->getModifiedTime( $context ) );
258 - }
259 - // Modules with dependencies but no group pass three arguments (name, timestamp, dependencies)
260 - // to mediaWiki.loader.register()
261 - else if ( $module->getGroup() === null ) {
262 - $registrations[] = array(
263 - $name, $module->getModifiedTime( $context ), $module->getDependencies() );
264 - }
265 - // Modules with dependencies pass four arguments (name, timestamp, dependencies, group)
266 - // to mediaWiki.loader.register()
267 - else {
268 - $registrations[] = array(
269 - $name, $module->getModifiedTime( $context ), $module->getDependencies(), $module->getGroup() );
270 - }
271 - }
272 - }
273 - $out = $scripts . "mediaWiki.loader.register( " . FormatJson::encode( $registrations ) . " );\n";
274 - wfProfileOut( __METHOD__ );
275 - return $out;
276 - }
277 -
278 - /**
279239 * Get the highest modification time of all modules, based on a given
280240 * combination of language code, skin name and debug mode flag.
281241 *
——@@ -356,99 +316,125 @@
357317 return;
358318 }
359319
360 - // Use output buffering
361 - ob_start();
362 -
363320 // Pre-fetch blobs
364321 $blobs = $context->shouldIncludeMessages() ?
365322 MessageBlobStore::get( $modules, $context->getLanguage() ) : array();
366323
367324 // Generate output
 325+ $out = '';
368326 foreach ( $modules as $name ) {
369327 wfProfileIn( __METHOD__ . '-' . $name );
 328+
370329 // Scripts
371330 $scripts = '';
372 -
373331 if ( $context->shouldIncludeScripts() ) {
374332 $scripts .= self::$modules[$name]->getScript( $context ) . "\n";
375333 }
376334
377335 // Styles
378336 $styles = array();
379 -
380337 if (
381 - $context->shouldIncludeStyles()
382 - && ( count( $styles = self::$modules[$name]->getStyles( $context ) ) )
 338+ $context->shouldIncludeStyles() &&
 339+ ( count( $styles = self::$modules[$name]->getStyles( $context ) ) )
383340 ) {
384 - foreach ( $styles as $media => $style ) {
385 - if ( self::$modules[$name]->getFlip( $context ) ) {
 341+ // Flip CSS on a per-module basis
 342+ if ( self::$modules[$name]->getFlip( $context ) ) {
 343+ foreach ( $styles as $media => $style ) {
386344 $styles[$media] = self::filter( 'flip-css', $style );
387345 }
388 - if ( !$context->getDebug() ) {
389 - $styles[$media] = self::filter( 'minify-css', $style );
390 - }
391346 }
392347 }
393348
394349 // Messages
395350 $messages = isset( $blobs[$name] ) ? $blobs[$name] : '{}';
396351
397 - // Output
398 - if ( $context->getOnly() === 'styles' ) {
399 - if ( $context->getDebug() ) {
400 - echo "/* $name */\n";
401 - foreach ( $styles as $media => $style ) {
402 - echo "@media $media {\n" . str_replace( "\n", "\n\t", "\t" . $style ) . "\n}\n";
403 - }
404 - } else {
405 - foreach ( $styles as $media => $style ) {
406 - if ( strlen( $style ) ) {
407 - echo "@media $media{" . $style . "}";
 352+ // Append output
 353+ switch ( $context->getOnly() ) {
 354+ case 'scripts':
 355+ $out .= $scripts;
 356+ break;
 357+ case 'styles':
 358+ $out .= self::makeCombinedStyles( $styles );
 359+ break;
 360+ case 'messages':
 361+ $out .= self::makeMessageSetScript( $messages );
 362+ break;
 363+ default:
 364+ // Minify CSS, unless in debug mode, before embedding in implment script
 365+ if ( !$context->getDebug() ) {
 366+ foreach ( $styles as $media => $style ) {
 367+ $styles[$media] = self::filter( 'minify-css', $style );
408368 }
409369 }
410 - }
411 - } else if ( $context->getOnly() === 'scripts' ) {
412 - echo $scripts;
413 - } else if ( $context->getOnly() === 'messages' ) {
414 - echo "mediaWiki.msg.set( $messages );\n";
415 - } else {
416 - if ( count( $styles ) ) {
417 - $styles = FormatJson::encode( $styles );
418 - } else {
419 - $styles = 'null';
420 - }
421 - echo "mediaWiki.loader.implement( '$name', function() {{$scripts}},\n$styles,\n$messages );\n";
 370+ $out .= self::makeLoaderImplementScript( $name, $scripts, $styles, $messages );
 371+ break;
422372 }
 373+
423374 wfProfileOut( __METHOD__ . '-' . $name );
424375 }
425376
426 - // Update the status of script-only modules
427 - if ( $context->getOnly() === 'scripts' && !in_array( 'startup', $modules ) ) {
428 - $statuses = array();
429 -
430 - foreach ( $modules as $name ) {
431 - $statuses[$name] = 'ready';
432 - }
433 -
434 - $statuses = FormatJson::encode( $statuses );
435 - echo "mediaWiki.loader.state( $statuses );\n";
436 - }
437 -
438 - // Register missing modules
 377+ // Update module states
439378 if ( $context->shouldIncludeScripts() ) {
440 - foreach ( $missing as $name ) {
441 - echo "mediaWiki.loader.register( '$name', null, 'missing' );\n";
 379+ // Set the state of modules loaded as only scripts to ready
 380+ if ( count( $modules ) && $context->getOnly() === 'scripts' && !in_array( 'startup', $modules ) ) {
 381+ $out .= self::makeLoaderStateScript( array_fill_keys( $modules, 'ready' ) );
442382 }
 383+ // Set the state of modules which were requested but unavailable as missing
 384+ if ( count( $missing ) ) {
 385+ $out .= self::makeLoaderStateScript( array_fill_keys( $missing, 'missing' ) );
 386+ }
443387 }
444388
445 - // Output the appropriate header
446 - if ( $context->getOnly() !== 'styles' ) {
447 - if ( $context->getDebug() ) {
448 - ob_end_flush();
 389+ // Send output
 390+ if ( $context->getDebug() ) {
 391+ echo $out;
 392+ } else {
 393+ if ( $context->getOnly() === 'styles' ) {
 394+ echo self::filter( 'minify-css', $out );
449395 } else {
450 - echo self::filter( 'minify-js', ob_get_clean() );
 396+ echo self::filter( 'minify-js', $out );
451397 }
452398 }
 399+
453400 wfProfileOut( __METHOD__ );
454401 }
 402+
 403+ public static function makeLoaderImplementScript( $name, $scripts, $styles, $messages ) {
 404+ if ( is_array( $scripts ) ) {
 405+ $scripts = implode( $scripts, "\n" );
 406+ }
 407+ if ( is_array( $styles ) ) {
 408+ $styles = count( $styles ) ? FormatJson::encode( $styles ) : 'null';
 409+ }
 410+ if ( is_array( $messages ) ) {
 411+ $messages = count( $messages ) ? FormatJson::encode( $messages ) : 'null';
 412+ }
 413+ return "mediaWiki.loader.implement( '$name', function() {{$scripts}},\n$styles,\n$messages );\n";
 414+ }
 415+
 416+ public static function makeMessageSetScript( $messages ) {
 417+ if ( is_array( $messages ) ) {
 418+ $messages = count( $messages ) ? FormatJson::encode( $messages ) : 'null';
 419+ }
 420+ return "mediaWiki.msg.set( $messages );\n";
 421+ }
 422+
 423+ public static function makeCombinedStyles( array $styles ) {
 424+ $out = '';
 425+ foreach ( $styles as $media => $style ) {
 426+ $out .= "@media $media {\n" . str_replace( "\n", "\n\t", "\t" . $style ) . "\n}\n";
 427+ }
 428+ return $out;
 429+ }
 430+
 431+ public static function makeLoaderStateScript( $name, $state = null ) {
 432+ if ( is_array( $name ) ) {
 433+ $statuses = FormatJson::encode( $name );
 434+ return "mediaWiki.loader.state( $statuses );\n";
 435+ } else {
 436+ $name = Xml::escapeJsString( $name );
 437+ $name = Xml::escapeJsString( $state );
 438+ return "mediaWiki.loader.state( '$name', '$state' );\n";
 439+ }
 440+ }
455441 }
Index: trunk/phase3/includes/ResourceLoaderModule.php
——@@ -26,6 +26,7 @@
2727 * Abstraction for resource loader modules, with name registration and maxage functionality.
2828 */
2929 abstract class ResourceLoaderModule {
 30+
3031 /* Protected Members */
3132
3233 protected $name = null;
——@@ -1038,25 +1039,66 @@
10391040 return $vars;
10401041 }
10411042
 1043+ /**
 1044+ * Gets registration code for all modules
 1045+ *
 1046+ * @param $context ResourceLoaderContext object
 1047+ * @return String: JavaScript code for registering all modules with the client loader
 1048+ */
 1049+ public static function getModuleRegistrations( ResourceLoaderContext $context ) {
 1050+ wfProfileIn( __METHOD__ );
 1051+
 1052+ $scripts = '';
 1053+ $registrations = array();
 1054+ foreach ( ResourceLoader::getModules() as $name => $module ) {
 1055+ // Support module loader scripts
 1056+ if ( ( $loader = $module->getLoaderScript() ) !== false ) {
 1057+ $deps = FormatJson::encode( $module->getDependencies() );
 1058+ $group = FormatJson::encode( $module->getGroup() );
 1059+ $version = wfTimestamp( TS_ISO_8601, round( $module->getModifiedTime( $context ), -2 ) );
 1060+ $scripts .= "( function( name, version, dependencies ) { $loader } )\n" .
 1061+ "( '$name', '$version', $deps, $group );\n";
 1062+ }
 1063+ // Automatically register module
 1064+ else {
 1065+ // Modules without dependencies or a group pass two arguments (name, timestamp) to
 1066+ // mediaWiki.loader.register()
 1067+ if ( !count( $module->getDependencies() && $module->getGroup() === null ) ) {
 1068+ $registrations[] = array( $name, $module->getModifiedTime( $context ) );
 1069+ }
 1070+ // Modules with dependencies but no group pass three arguments (name, timestamp, dependencies)
 1071+ // to mediaWiki.loader.register()
 1072+ else if ( $module->getGroup() === null ) {
 1073+ $registrations[] = array(
 1074+ $name, $module->getModifiedTime( $context ), $module->getDependencies() );
 1075+ }
 1076+ // Modules with dependencies pass four arguments (name, timestamp, dependencies, group)
 1077+ // to mediaWiki.loader.register()
 1078+ else {
 1079+ $registrations[] = array(
 1080+ $name, $module->getModifiedTime( $context ), $module->getDependencies(), $module->getGroup() );
 1081+ }
 1082+ }
 1083+ }
 1084+ $out = $scripts . "mediaWiki.loader.register( " . FormatJson::encode( $registrations ) . " );";
 1085+
 1086+ wfProfileOut( __METHOD__ );
 1087+ return $out;
 1088+ }
 1089+
10421090 /* Methods */
10431091
10441092 public function getScript( ResourceLoaderContext $context ) {
10451093 global $IP, $wgLoadScript;
10461094
10471095 $scripts = file_get_contents( "$IP/resources/startup.js" );
1048 -
10491096 if ( $context->getOnly() === 'scripts' ) {
10501097 // Get all module registrations
1051 - $registration = ResourceLoader::getModuleRegistrations( $context );
 1098+ $registration = self::getModuleRegistrations( $context );
10521099 // Build configuration
10531100 $config = FormatJson::encode( $this->getConfig( $context ) );
10541101 // Add a well-known start-up function
1055 - $scripts .= <<<JAVASCRIPT
1056 -window.startUp = function() {
1057 - $registration
1058 - mediaWiki.config.set( $config );
1059 -};
1060 -JAVASCRIPT;
 1102+ $scripts .= "window.startUp = function() {\n\t$registration\n\tmediaWiki.config.set( $config );\n};\n";
10611103 // Build load query for jquery and mediawiki modules
10621104 $query = array(
10631105 'modules' => implode( '|', array( 'jquery', 'mediawiki' ) ),
——@@ -1074,7 +1116,7 @@
10751117 // Build HTML code for loading jquery and mediawiki modules
10761118 $loadScript = Html::linkedScript( $wgLoadScript . '?' . wfArrayToCGI( $query ) );
10771119 // Add code to add jquery and mediawiki loading code; only if the current client is compatible
1078 - $scripts .= "if ( isCompatible() ) { document.write( " . FormatJson::encode( $loadScript ) . "); }\n";
 1120+ $scripts .= "if ( isCompatible() ) {\n\tdocument.write( " . FormatJson::encode( $loadScript ) . ");\n}\n";
10791121 // Delete the compatible function - it's not needed anymore
10801122 $scripts .= "delete window['isCompatible'];\n";
10811123 }
——@@ -1090,10 +1132,10 @@
10911133 return $this->modifiedTime[$hash];
10921134 }
10931135 $this->modifiedTime[$hash] = filemtime( "$IP/resources/startup.js" );
 1136+
10941137 // ATTENTION!: Because of the line above, this is not going to cause infinite recursion - think carefully
10951138 // before making changes to this code!
1096 - $this->modifiedTime[$hash] = ResourceLoader::getHighestModifiedTime( $context );
1097 - return $this->modifiedTime[$hash];
 1139+ return $this->modifiedTime[$hash] = ResourceLoader::getHighestModifiedTime( $context );
10981140 }
10991141
11001142 public function getFlip( $context ) {
Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -735,9 +735,10 @@
736736 }
737737 return;
738738 }
739 - if ( module in registry ) {
740 - registry[module].state = state;
 739+ if ( !( module in registry ) ) {
 740+ that.register( module );
741741 }
 742+ registry[module].state = state;
742743 };
743744
744745 /**

Follow-up revisions

RevisionCommit summaryAuthorDate
r73686* Fixed bug #25281 by adding special treatment for modules in the "private" g...tparscal22:10, 24 September 2010
r74115Fixed typo in r73673, thank for spotting that Roan!tparscal22:06, 1 October 2010

Comments

#Comment by Nikerabbit (talk | contribs)   21:28, 24 September 2010 
+ // Minify CSS, unless in debug mode, before embedding in implment script

implment?

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:07, 1 October 2010

Yeah, this has been cleaned up now.

#Comment by Catrope (talk | contribs)   21:26, 1 October 2010 
+			$name = Xml::escapeJsString( $name );
+			$name = Xml::escapeJsString( $state );

That looks wrong.

#Comment by Trevor Parscal (WMF) (talk | contribs)   22:06, 1 October 2010

Yes, thank you for spotting this! r74115 fixes it.

Status & tagging log