r73204 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r73203‎ | r73204 | r73205 >
Date:11:45, 17 September 2010
Author:tstarling
Status:ok
Tags:
Comment:
Minor resource loader changes:
* Broke some long lines, converted some overly complex ternary operators to if()
* Added lots of line breaks into the output, for easier debugging.
* Added profiling for various functions.
* wfGetDb -> wfGetDB
* Fixed escaping in ResourceLoaderStartUpModule::getScript(), escape for JS rather than assuming Html::linkedScript() won't have any bad characters in it.
Modified paths:
  • /trunk/phase3/includes/MessageBlobStore.php (modified) (history)
  • /trunk/phase3/includes/OutputPage.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoader.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoaderContext.php (modified) (history)
  • /trunk/phase3/includes/ResourceLoaderModule.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/ResourceLoaderContext.php
@@ -20,7 +20,8 @@
2121 */
2222
2323 /**
24 - * Object passed around to modules which contains information about the state of a specific loader request
 24+ * Object passed around to modules which contains information about the state
 25+ * of a specific loader request
2526 */
2627 class ResourceLoaderContext {
2728 /* Protected Members */
@@ -115,9 +116,12 @@
116117 }
117118
118119 public function getHash() {
119 - return isset( $this->hash ) ?
120 - $this->hash : $this->hash = implode( '|', array(
121 - $this->language, $this->direction, $this->skin, $this->user, $this->debug, $this->only, $this->version
 120+ if ( isset( $this->hash ) ) {
 121+ $this->hash = implode( '|', array(
 122+ $this->language, $this->direction, $this->skin, $this->user,
 123+ $this->debug, $this->only, $this->version
122124 ) );
 125+ }
 126+ return $this->hash;
123127 }
124 -}
\ No newline at end of file
 128+}
Index: trunk/phase3/includes/OutputPage.php
@@ -2325,9 +2325,9 @@
23262326 ksort( $query );
23272327 // Automatically select style/script elements
23282328 if ( $only === 'styles' ) {
2329 - $links .= Html::linkedStyle( wfAppendQuery( $wgLoadScript, $query ) );
 2329+ $links .= Html::linkedStyle( wfAppendQuery( $wgLoadScript, $query ) ) . "\n";
23302330 } else {
2331 - $links .= Html::linkedScript( wfAppendQuery( $wgLoadScript, $query ) );
 2331+ $links .= Html::linkedScript( wfAppendQuery( $wgLoadScript, $query ) ) . "\n";
23322332 }
23332333 }
23342334 }
@@ -2378,8 +2378,8 @@
23792379 if ( $this->getModules() ) {
23802380 $modules = FormatJson::encode( $this->getModules() );
23812381 $scripts .= Html::inlineScript(
2382 - "if ( mediaWiki !== undefined ) { mediaWiki.loader.load( {$modules} ); mediaWiki.loader.go(); }"
2383 - );
 2382+ "if ( mediaWiki !== undefined ) { mediaWiki.loader.load( {$modules} ); mediaWiki.loader.go(); }\n"
 2383+ ) . "\n";
23842384 }
23852385
23862386 // Add user JS if enabled - trying to load user.options as a bundle if possible
Index: trunk/phase3/includes/ResourceLoader.php
@@ -41,10 +41,13 @@
4242
4343 // Safety check - this should never be called more than once
4444 if ( !self::$initialized ) {
45 - // This needs to be first, because hooks might call ResourceLoader public interfaces which will call this
 45+ wfProfileIn( __METHOD__ );
 46+ // This needs to be first, because hooks might call ResourceLoader
 47+ // public interfaces which will call this
4648 self::$initialized = true;
4749 self::register( include( "$IP/resources/Resources.php" ) );
4850 wfRunHooks( 'ResourceLoaderRegisterModules' );
 51+ wfProfileOut( __METHOD__ );
4952 }
5053 }
5154
@@ -53,14 +56,17 @@
5457 *
5558 * @param $filter String: name of filter to run
5659 * @param $data String: text to filter, such as JavaScript or CSS text
57 - * @param $file String: path to file being filtered, (optional: only required for CSS to resolve paths)
 60+ * @param $file String: path to file being filtered, (optional: only required
 61+ * for CSS to resolve paths)
5862 * @return String: filtered data
5963 */
6064 protected static function filter( $filter, $data ) {
6165 global $wgMemc;
 66+ wfProfileIn( __METHOD__ );
6267
6368 // For empty or whitespace-only things, don't do any processing
6469 if ( trim( $data ) === '' ) {
 70+ wfProfileOut( __METHOD__ );
6571 return $data;
6672 }
6773
@@ -69,6 +75,7 @@
7076 $cached = $wgMemc->get( $key );
7177
7278 if ( $cached !== false && $cached !== null ) {
 79+ wfProfileOut( __METHOD__ );
7380 return $cached;
7481 }
7582
@@ -86,6 +93,7 @@
8794 break;
8895 default:
8996 // Don't cache anything, just pass right through
 97+ wfProfileOut( __METHOD__ );
9098 return $data;
9199 }
92100 } catch ( Exception $exception ) {
@@ -95,6 +103,7 @@
96104 // Save to memcached
97105 $wgMemc->set( $key, $result );
98106
 107+ wfProfileOut( __METHOD__ );
99108 return $result;
100109 }
101110
@@ -103,18 +112,21 @@
104113 /**
105114 * Registers a module with the ResourceLoader system.
106115 *
107 - * Note that registering the same object under multiple names is not supported and may silently fail in all
108 - * kinds of interesting ways.
 116+ * Note that registering the same object under multiple names is not supported
 117+ * and may silently fail in all kinds of interesting ways.
109118 *
110119 * @param $name Mixed: string of name of module or array of name/object pairs
111 - * @param $object ResourceLoaderModule: module object (optional when using multiple-registration calling style)
112 - * @return Boolean: false if there were any errors, in which case one or more modules were not registered
 120+ * @param $object ResourceLoaderModule: module object (optional when using
 121+ * multiple-registration calling style)
 122+ * @return Boolean: false if there were any errors, in which case one or more
 123+ * modules were not registered
113124 *
114 - * @todo We need much more clever error reporting, not just in detailing what happened, but in bringing errors to
115 - * the client in a way that they can easily see them if they want to, such as by using FireBug
 125+ * @todo We need much more clever error reporting, not just in detailing what
 126+ * happened, but in bringing errors to the client in a way that they can
 127+ * easily see them if they want to, such as by using FireBug
116128 */
117129 public static function register( $name, ResourceLoaderModule $object = null ) {
118 -
 130+ wfProfileIn( __METHOD__ );
119131 self::initialize();
120132
121133 // Allow multiple modules to be registered in one call
@@ -123,6 +135,7 @@
124136 self::register( $key, $value );
125137 }
126138
 139+ wfProfileOut( __METHOD__ );
127140 return;
128141 }
129142
@@ -134,6 +147,7 @@
135148 // Attach module
136149 self::$modules[$name] = $object;
137150 $object->setName( $name );
 151+ wfProfileOut( __METHOD__ );
138152 }
139153
140154 /**
@@ -162,13 +176,14 @@
163177 }
164178
165179 /**
166 - * Gets registration code for all modules, except pre-registered ones listed in self::$preRegisteredModules
 180+ * Gets registration code for all modules, except pre-registered ones listed in
 181+ * self::$preRegisteredModules
167182 *
168183 * @param $context ResourceLoaderContext object
169184 * @return String: JavaScript code for registering all modules with the client loader
170185 */
171186 public static function getModuleRegistrations( ResourceLoaderContext $context ) {
172 -
 187+ wfProfileIn( __METHOD__ );
173188 self::initialize();
174189
175190 $scripts = '';
@@ -178,27 +193,34 @@
179194 // Support module loader scripts
180195 if ( ( $loader = $module->getLoaderScript() ) !== false ) {
181196 $deps = FormatJson::encode( $module->getDependencies() );
182 - $version = wfTimestamp( TS_ISO_8601, round( $module->getModifiedTime( $context ), -2 ) );
183 - $scripts .= "( function( name, version, dependencies ) { $loader } )( '$name', '$version', $deps );";
 197+ $version = wfTimestamp( TS_ISO_8601,
 198+ round( $module->getModifiedTime( $context ), -2 ) );
 199+ $scripts .= "( function( name, version, dependencies ) { $loader } )\n" .
 200+ "( '$name', '$version', $deps );\n";
184201 }
185202 // Automatically register module
186203 else {
187 - // Modules without dependencies pass two arguments (name, timestamp) to mediaWiki.loader.register()
 204+ // Modules without dependencies pass two arguments (name, timestamp) to
 205+ // mediaWiki.loader.register()
188206 if ( !count( $module->getDependencies() ) ) {
189207 $registrations[] = array( $name, $module->getModifiedTime( $context ) );
190208 }
191 - // Modules with dependencies pass three arguments (name, timestamp, dependencies) to mediaWiki.loader.register()
 209+ // Modules with dependencies pass three arguments (name, timestamp, dependencies)
 210+ // to mediaWiki.loader.register()
192211 else {
193 - $registrations[] = array( $name, $module->getModifiedTime( $context ), $module->getDependencies() );
 212+ $registrations[] = array( $name, $module->getModifiedTime( $context ),
 213+ $module->getDependencies() );
194214 }
195215 }
196216 }
197 - return $scripts . "mediaWiki.loader.register( " . FormatJson::encode( $registrations ) . " );";
 217+ $out = $scripts . "mediaWiki.loader.register( " . FormatJson::encode( $registrations ) . " );\n";
 218+ wfProfileOut( __METHOD__ );
 219+ return $out;
198220 }
199221
200222 /**
201 - * Get the highest modification time of all modules, based on a given combination of language code,
202 - * skin name and debug mode flag.
 223+ * Get the highest modification time of all modules, based on a given
 224+ * combination of language code, skin name and debug mode flag.
203225 *
204226 * @param $context ResourceLoaderContext object
205227 * @return Integer: UNIX timestamp
@@ -225,6 +247,7 @@
226248 global $wgResourceLoaderVersionedClientMaxage, $wgResourceLoaderVersionedServerMaxage;
227249 global $wgResourceLoaderUnversionedServerMaxage, $wgResourceLoaderUnversionedClientMaxage;
228250
 251+ wfProfileIn( __METHOD__ );
229252 self::initialize();
230253
231254 // Split requested modules into two groups, modules and missing
@@ -239,22 +262,27 @@
240263 }
241264 }
242265
243 - // If a version wasn't specified we need a shorter expiry time for updates to propagate to clients quickly
 266+ // If a version wasn't specified we need a shorter expiry time for updates to
 267+ // propagate to clients quickly
244268 if ( is_null( $context->getVersion() ) ) {
245269 $maxage = $wgResourceLoaderUnversionedClientMaxage;
246270 $smaxage = $wgResourceLoaderUnversionedServerMaxage;
247271 }
248 - // If a version was specified we can use a longer expiry time since changing version numbers causes cache misses
 272+ // If a version was specified we can use a longer expiry time since changing
 273+ // version numbers causes cache misses
249274 else {
250275 $maxage = $wgResourceLoaderVersionedClientMaxage;
251276 $smaxage = $wgResourceLoaderVersionedServerMaxage;
252277 }
253278
254 - // To send Last-Modified and support If-Modified-Since, we need to detect the last modified time
 279+ // To send Last-Modified and support If-Modified-Since, we need to detect
 280+ // the last modified time
 281+ wfProfileIn( __METHOD__.'-getModifiedTime' );
255282 $mtime = 1;
256283 foreach ( $modules as $name ) {
257284 $mtime = max( $mtime, self::$modules[$name]->getModifiedTime( $context ) );
258285 }
 286+ wfProfileOut( __METHOD__.'-getModifiedTime' );
259287
260288 header( 'Content-Type: ' . ( $context->getOnly() === 'styles' ? 'text/css' : 'text/javascript' ) );
261289 header( 'Last-Modified: ' . wfTimestamp( TS_RFC2822, $mtime ) );
@@ -266,6 +294,7 @@
267295 if ( $ims !== false && $mtime >= wfTimestamp( TS_UNIX, $ims ) ) {
268296 header( 'HTTP/1.0 304 Not Modified' );
269297 header( 'Status: 304 Not Modified' );
 298+ wfProfileOut( __METHOD__ );
270299 return;
271300 }
272301
@@ -278,18 +307,20 @@
279308
280309 // Generate output
281310 foreach ( $modules as $name ) {
 311+ wfProfileIn( __METHOD__ . '-' . $name );
282312 // Scripts
283313 $scripts = '';
284314
285315 if ( $context->shouldIncludeScripts() ) {
286 - $scripts .= self::$modules[$name]->getScript( $context );
 316+ $scripts .= self::$modules[$name]->getScript( $context ) . "\n";
287317 }
288318
289319 // Styles
290320 $styles = array();
291321
292322 if (
293 - $context->shouldIncludeStyles() && ( count( $styles = self::$modules[$name]->getStyles( $context ) ) )
 323+ $context->shouldIncludeStyles()
 324+ && ( count( $styles = self::$modules[$name]->getStyles( $context ) ) )
294325 ) {
295326 foreach ( $styles as $media => $style ) {
296327 if ( self::$modules[$name]->getFlip( $context ) ) {
@@ -330,6 +361,7 @@
331362 }
332363 echo "mediaWiki.loader.implement( '$name', function() {{$scripts}},\n$styles,\n$messages );\n";
333364 }
 365+ wfProfileOut( __METHOD__ . '-' . $name );
334366 }
335367
336368 // Update the status of script-only modules
@@ -359,5 +391,6 @@
360392 echo self::filter( 'minify-js', ob_get_clean() );
361393 }
362394 }
 395+ wfProfileOut( __METHOD__ );
363396 }
364 -}
\ No newline at end of file
 397+}
Index: trunk/phase3/includes/MessageBlobStore.php
@@ -37,7 +37,9 @@
3838 */
3939 public static function get( $modules, $lang ) {
4040 // TODO: Invalidate blob when module touched
 41+ wfProfileIn( __METHOD__ );
4142 if ( !count( $modules ) ) {
 43+ wfProfileOut( __METHOD__ );
4244 return array();
4345 }
4446 // Try getting from the DB first
@@ -52,6 +54,7 @@
5355 }
5456 }
5557
 58+ wfProfileOut( __METHOD__ );
5659 return $blobs;
5760 }
5861
@@ -115,7 +118,8 @@
116119 * Update all message blobs for a given module.
117120 * @param $module string Module name
118121 * @param $lang string Language code (optional)
119 - * @return mixed If $lang is set, the new message blob for that language is returned if present. Otherwise, null is returned.
 122+ * @return mixed If $lang is set, the new message blob for that language is
 123+ * returned if present. Otherwise, null is returned.
120124 */
121125 public static function updateModule( $module, $lang = null ) {
122126 $retval = null;
Index: trunk/phase3/includes/ResourceLoaderModule.php
@@ -380,7 +380,7 @@
381381 // Only store if modified
382382 if ( $files !== $this->getFileDependencies( $context->getSkin() ) ) {
383383 $encFiles = FormatJson::encode( $files );
384 - $dbw = wfGetDb( DB_MASTER );
 384+ $dbw = wfGetDB( DB_MASTER );
385385 $dbw->replace( 'module_deps',
386386 array( array( 'md_module', 'md_skin' ) ), array(
387387 'md_module' => $this->getName(),
@@ -430,6 +430,7 @@
431431 if ( isset( $this->modifiedTime[$context->getHash()] ) ) {
432432 return $this->modifiedTime[$context->getHash()];
433433 }
 434+ wfProfileIn( __METHOD__ );
434435
435436 // Sort of nasty way we can get a flat list of files depended on by all styles
436437 $styles = array();
@@ -454,13 +455,16 @@
455456 $this->loaders,
456457 $this->getFileDependencies( $context->getSkin() )
457458 );
 459+ wfProfileIn( __METHOD__.'-filemtime' );
458460 $filesMtime = max( array_map( 'filemtime', array_map( array( __CLASS__, 'remapFilename' ), $files ) ) );
 461+ wfProfileOut( __METHOD__.'-filemtime' );
459462 // Only get the message timestamp if there are messages in the module
460463 $msgBlobMtime = 0;
461464 if ( count( $this->messages ) ) {
462465 // Get the mtime of the message blob
463 - // TODO: This timestamp is queried a lot and queried separately for each module. Maybe it should be put in memcached?
464 - $dbr = wfGetDb( DB_SLAVE );
 466+ // TODO: This timestamp is queried a lot and queried separately for each module.
 467+ // Maybe it should be put in memcached?
 468+ $dbr = wfGetDB( DB_SLAVE );
465469 $msgBlobMtime = $dbr->selectField( 'msg_resource', 'mr_timestamp', array(
466470 'mr_resource' => $this->getName(),
467471 'mr_lang' => $context->getLanguage()
@@ -469,6 +473,7 @@
470474 $msgBlobMtime = $msgBlobMtime ? wfTimestamp( TS_UNIX, $msgBlobMtime ) : 0;
471475 }
472476 $this->modifiedTime[$context->getHash()] = max( $filesMtime, $msgBlobMtime );
 477+ wfProfileOut( __METHOD__ );
473478 return $this->modifiedTime[$context->getHash()];
474479 }
475480
@@ -576,7 +581,7 @@
577582 $deps = $wgMemc->get( $key );
578583
579584 if ( !$deps ) {
580 - $dbr = wfGetDb( DB_SLAVE );
 585+ $dbr = wfGetDB( DB_SLAVE );
581586 $deps = $dbr->selectField( 'module_deps', 'md_deps', array(
582587 'md_module' => $this->getName(),
583588 'md_skin' => $skin,
@@ -601,7 +606,12 @@
602607 * @return String: concatenated contents of $files
603608 */
604609 protected static function concatScripts( $files ) {
605 - return implode( "\n", array_map( 'file_get_contents', array_map( array( __CLASS__, 'remapFilename' ), array_unique( (array) $files ) ) ) );
 610+ return implode( "\n",
 611+ array_map(
 612+ 'file_get_contents',
 613+ array_map(
 614+ array( __CLASS__, 'remapFilename' ),
 615+ array_unique( (array) $files ) ) ) );
606616 }
607617
608618 protected static function organizeFilesByOption( $files, $option, $default ) {
@@ -636,7 +646,10 @@
637647 $styles = self::organizeFilesByOption( $styles, 'media', 'all' );
638648 foreach ( $styles as $media => $files ) {
639649 $styles[$media] =
640 - implode( "\n", array_map( array( __CLASS__, 'remapStyle' ), array_unique( (array) $files ) ) );
 650+ implode( "\n",
 651+ array_map(
 652+ array( __CLASS__, 'remapStyle' ),
 653+ array_unique( (array) $files ) ) );
641654 }
642655 return $styles;
643656 }
@@ -841,7 +854,11 @@
842855
843856 public function getScript( ResourceLoaderContext $context ) {
844857 $user = User::newFromName( $context->getUser() );
845 - $options = FormatJson::encode( $user instanceof User ? $user->getOptions() : User::getDefaultOptions() );
 858+ if ( $user instanceof User ) {
 859+ $options = FormatJson::encode( $user->getOptions() );
 860+ } else {
 861+ $options = FormatJson::encode( User::getDefaultOptions() );
 862+ }
846863 return "mediaWiki.user.options.set( $options );";
847864 }
848865
@@ -894,9 +911,11 @@
895912 /* Protected Methods */
896913
897914 protected function getConfig( $context ) {
898 - global $wgLoadScript, $wgScript, $wgStylePath, $wgScriptExtension, $wgArticlePath, $wgScriptPath, $wgServer,
899 - $wgContLang, $wgBreakFrames, $wgVariantArticlePath, $wgActionPaths, $wgUseAjax, $wgVersion,
900 - $wgEnableAPI, $wgEnableWriteAPI, $wgDBname, $wgEnableMWSuggest, $wgSitename, $wgFileExtensions;
 915+ global $wgLoadScript, $wgScript, $wgStylePath, $wgScriptExtension,
 916+ $wgArticlePath, $wgScriptPath, $wgServer, $wgContLang, $wgBreakFrames,
 917+ $wgVariantArticlePath, $wgActionPaths, $wgUseAjax, $wgVersion,
 918+ $wgEnableAPI, $wgEnableWriteAPI, $wgDBname, $wgEnableMWSuggest,
 919+ $wgSitename, $wgFileExtensions;
901920
902921 // Pre-process information
903922 $separatorTransTable = $wgContLang->separatorTransformTable();
@@ -965,7 +984,12 @@
966985 // Build configuration
967986 $config = FormatJson::encode( $this->getConfig( $context ) );
968987 // Add a well-known start-up function
969 - $scripts .= "window.startUp = function() { $registration mediaWiki.config.set( $config ); };";
 988+ $scripts .= <<<JAVASCRIPT
 989+window.startUp = function() {
 990+ $registration
 991+ mediaWiki.config.set( $config );
 992+};
 993+JAVASCRIPT;
970994 // Build load query for jquery and mediawiki modules
971995 $query = array(
972996 'modules' => implode( '|', array( 'jquery', 'mediawiki' ) ),
@@ -983,9 +1007,9 @@
9841008 // Build HTML code for loading jquery and mediawiki modules
9851009 $loadScript = Html::linkedScript( $wgLoadScript . '?' . wfArrayToCGI( $query ) );
9861010 // Add code to add jquery and mediawiki loading code; only if the current client is compatible
987 - $scripts .= "if ( isCompatible() ) { document.write( '$loadScript' ); }";
 1011+ $scripts .= "if ( isCompatible() ) { document.write( " . FormatJson::encode( $loadScript ) . "); }\n";
9881012 // Delete the compatible function - it's not needed anymore
989 - $scripts .= "delete window['isCompatible'];";
 1013+ $scripts .= "delete window['isCompatible'];\n";
9901014 }
9911015
9921016 return $scripts;

Follow-up revisions

RevisionCommit summaryAuthorDate
r82465Fix logic error in r73204 (!) causing ResourceLoaderContext::getHash() to alw...catrope16:48, 19 February 2011

Status & tagging log