r83658 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r83657‎ | r83658 | r83659 >
Date:18:49, 10 March 2011
Author:krinkle
Status:resolved (Comments)
Tags:
Comment:
Fixing minor issues with mw.loader
* Using $.isArray/isFunction instead of "instanceof" or typeof.
** "instanceof Array" can throw SyntaxErrors in some cases
** Some non-functions return 'function' as their type
* JSHint:
** using === to compare to null (faster and safer)
** Missing semicolons
** Mixed spaces with tabs
* Added mw.log call when module is loaded
* Added support for custom prefix to mw.log (the mw.config value for 'mw.log.prefix' is pretty ugly as it sets it globally, when calling a function later again it's no longer the same. Perhaps just get rid of it)
Modified paths:
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.log.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/mediawiki/mediawiki.js
@@ -4,10 +4,10 @@
55
66 jQuery.extend({
77 trimLeft : function( str ) {
8 - return str == null ? '' : str.toString().replace( /^\s+/, '' );
 8+ return str === null ? '' : str.toString().replace( /^\s+/, '' );
99 },
1010 trimRight : function( str ) {
11 - return str == null ?
 11+ return str === null ?
1212 '' : str.toString().replace( /\s+$/, '' );
1313 },
1414 ucFirst : function( str ) {
@@ -300,7 +300,7 @@
301301 Message.prototype.escaped = function() {
302302 this.format = 'escaped';
303303 return this.toString();
304 - }
 304+ };
305305
306306 /**
307307 * Checks if message exists
@@ -376,7 +376,7 @@
377377 */
378378 this.sessionId = function () {
379379 var sessionId = $.cookie( 'mediaWiki.user.sessionId' );
380 - if ( typeof sessionId == 'undefined' || sessionId == null ) {
 380+ if ( typeof sessionId == 'undefined' || sessionId === null ) {
381381 sessionId = generateId();
382382 $.cookie( 'mediaWiki.user.sessionId', sessionId, { 'expires': null, 'path': '/' } );
383383 }
@@ -402,7 +402,7 @@
403403 return name;
404404 }
405405 var id = $.cookie( 'mediaWiki.user.id' );
406 - if ( typeof id == 'undefined' || id == null ) {
 406+ if ( typeof id == 'undefined' || id === null ) {
407407 id = generateId();
408408 }
409409 // Set cookie if not set, or renew it if already set
@@ -489,16 +489,16 @@
490490 * mediawiki.
491491 *
492492 * Format:
493 - * {
494 - * 'moduleName': {
495 - * 'dependencies': ['required module', 'required module', ...], (or) function() {}
496 - * 'state': 'registered', 'loading', 'loaded', 'ready', or 'error'
497 - * 'script': function() {},
498 - * 'style': 'css code string',
499 - * 'messages': { 'key': 'value' },
500 - * 'version': ############## (unix timestamp)
501 - * }
502 - * }
 493+ * {
 494+ * 'moduleName': {
 495+ * 'dependencies': ['required module', 'required module', ...], (or) function() {}
 496+ * 'state': 'registered', 'loading', 'loaded', 'ready', or 'error'
 497+ * 'script': function() {},
 498+ * 'style': 'css code string',
 499+ * 'messages': { 'key': 'value' },
 500+ * 'version': ############## (unix timestamp)
 501+ * }
 502+ * }
503503 */
504504 var registry = {};
505505 // List of modules which will be loaded as when ready
@@ -556,7 +556,7 @@
557557 throw new Error( 'Unknown dependency: ' + module );
558558 }
559559 // Resolves dynamic loader function and replaces it with its own results
560 - if ( typeof registry[module].dependencies === 'function' ) {
 560+ if ( $.isFunction( registry[module].dependencies ) ) {
561561 registry[module].dependencies = registry[module].dependencies();
562562 // Ensures the module's dependencies are always in an array
563563 if ( typeof registry[module].dependencies !== 'object' ) {
@@ -625,7 +625,7 @@
626626 states = [states];
627627 }
628628 // If called without a list of modules, build and use a list of all modules
629 - var list = [];
 629+ var list = [], module;
630630 if ( typeof modules === 'undefined' ) {
631631 modules = [];
632632 for ( module in registry ) {
@@ -659,6 +659,7 @@
660660 * @param module string module name to execute
661661 */
662662 function execute( module ) {
 663+ var _method = 'mw.loader::execute';
663664 if ( typeof registry[module] === 'undefined' ) {
664665 throw new Error( 'Module has not been registered yet: ' + module );
665666 } else if ( registry[module].state === 'registered' ) {
@@ -675,7 +676,7 @@
676677 new mediaWiki.html.Cdata( registry[module].style )
677678 ) );
678679 } else if ( typeof registry[module].style === 'object'
679 - && !( registry[module].style instanceof Array ) )
 680+ && !( $.isArray( registry[module].style ) ) )
680681 {
681682 for ( var media in registry[module].style ) {
682683 $marker.before( mediaWiki.html.element( 'style',
@@ -691,6 +692,7 @@
692693 // Execute script
693694 try {
694695 registry[module].script( jQuery );
 696+ mw.log( 'State ready: ' + module, _method )
695697 registry[module].state = 'ready';
696698 // Run jobs who's dependencies have just been met
697699 for ( var j = 0; j < jobs.length; j++ ) {
@@ -698,7 +700,7 @@
699701 filter( 'ready', jobs[j].dependencies ),
700702 jobs[j].dependencies ) )
701703 {
702 - if ( typeof jobs[j].ready === 'function' ) {
 704+ if ( $.isFunction( jobs[j].ready ) ) {
703705 jobs[j].ready();
704706 }
705707 jobs.splice( j, 1 );
@@ -706,7 +708,7 @@
707709 }
708710 }
709711 // Execute modules who's dependencies have just been met
710 - for ( r in registry ) {
 712+ for ( var r in registry ) {
711713 if ( registry[r].state == 'loaded' ) {
712714 if ( compare(
713715 filter( ['ready'], registry[r].dependencies ),
@@ -717,13 +719,13 @@
718720 }
719721 }
720722 } catch ( e ) {
721 - mediaWiki.log( 'Exception thrown by ' + module + ': ' + e.message );
 723+ mediaWiki.log( 'Exception thrown by ' + module + ': ' + e.message, _method );
722724 mediaWiki.log( e );
723725 registry[module].state = 'error';
724726 // Run error callbacks of jobs affected by this condition
725727 for ( var j = 0; j < jobs.length; j++ ) {
726728 if ( $.inArray( module, jobs[j].dependencies ) !== -1 ) {
727 - if ( typeof jobs[j].error === 'function' ) {
 729+ if ( $.isFunction( jobs[j].error ) ) {
728730 jobs[j].error();
729731 }
730732 jobs.splice( j, 1 );
@@ -863,7 +865,7 @@
864866 }
865867 // Load asynchronously after documument ready
866868 if ( ready ) {
867 - setTimeout( function() { $( 'body' ).append( request() ); }, 0 )
 869+ setTimeout( function() { $( 'body' ).append( request() ); }, 0 );
868870 } else {
869871 document.write( request() );
870872 }
@@ -898,12 +900,12 @@
899901 'state': 'registered',
900902 'group': typeof group === 'string' ? group : null,
901903 'dependencies': [],
902 - 'version': typeof version !== 'undefined' ? parseInt( version ) : 0
 904+ 'version': typeof version !== 'undefined' ? parseInt( version, 10 ) : 0
903905 };
904906 if ( typeof dependencies === 'string' ) {
905907 // Allow dependencies to be given as a single module name
906908 registry[module].dependencies = [dependencies];
907 - } else if ( typeof dependencies === 'object' || typeof dependencies === 'function' ) {
 909+ } else if ( typeof dependencies === 'object' || $.isFunction( dependencies ) ) {
908910 // Allow dependencies to be given as an array of module names
909911 // or a function which returns an array
910912 registry[module].dependencies = dependencies;
@@ -921,7 +923,7 @@
922924 mediaWiki.loader.register( module );
923925 }
924926 // Validate input
925 - if ( typeof script !== 'function' ) {
 927+ if ( !$.isFunction( script ) ) {
926928 throw new Error( 'script must be a function, not a ' + typeof script );
927929 }
928930 if ( typeof style !== 'undefined'
@@ -976,7 +978,7 @@
977979 // Validate input
978980 if ( typeof dependencies !== 'object' && typeof dependencies !== 'string' ) {
979981 throw new Error( 'dependencies must be a string or an array, not a ' +
980 - typeof dependencies )
 982+ typeof dependencies );
981983 }
982984 // Allow calling with a single dependency as a string
983985 if ( typeof dependencies === 'string' ) {
@@ -986,13 +988,13 @@
987989 dependencies = resolve( dependencies );
988990 // If all dependencies are met, execute ready immediately
989991 if ( compare( filter( ['ready'], dependencies ), dependencies ) ) {
990 - if ( typeof ready === 'function' ) {
 992+ if ( $.isFunction( ready ) ) {
991993 ready();
992994 }
993995 }
994996 // If any dependencies have errors execute error immediately
995997 else if ( filter( ['error'], dependencies ).length ) {
996 - if ( typeof error === 'function' ) {
 998+ if ( $.isFunction( error ) ) {
997999 error();
9981000 }
9991001 }
@@ -1016,7 +1018,7 @@
10171019 // Validate input
10181020 if ( typeof modules !== 'object' && typeof modules !== 'string' ) {
10191021 throw new Error( 'modules must be a string or an array, not a ' +
1020 - typeof modules )
 1022+ typeof modules );
10211023 }
10221024 // Allow calling with an external script or single dependency as a string
10231025 if ( typeof modules === 'string' ) {
@@ -1205,7 +1207,7 @@
12061208
12071209 /* Auto-register from pre-loaded startup scripts */
12081210
1209 -if ( typeof startUp === 'function' ) {
 1211+if ( $.isFunction( startUp ) ) {
12101212 startUp();
12111213 delete startUp;
12121214 }
Index: trunk/phase3/resources/mediawiki/mediawiki.log.js
@@ -15,9 +15,11 @@
1616 * @author Trevor Parscal <tparscal@wikimedia.org>
1717 * @param {string} string Message to output to console
1818 */
19 - mw.log = function( string ) {
 19+ mw.log = function( string, prefix ) {
2020 // Allow log messages to use a configured prefix
21 - if ( mw.config.exists( 'mw.log.prefix' ) ) {
 21+ if ( typeof prefix == 'string' ) {
 22+ string = prefix + '> ' + string;
 23+ } else if ( mw.config.exists( 'mw.log.prefix' ) ) {
2224 string = mw.config.get( 'mw.log.prefix' ) + '> ' + string;
2325 }
2426 // Try to use an existing console

Follow-up revisions

RevisionCommit summaryAuthorDate
r84275per r83658 CR: mw.log.prefix is for identifying windows, not context. Using s...krinkle22:16, 18 March 2011
r84276per r83658 CR: mw.log.prefix is for identifying windows, not context. Using s...krinkle22:18, 18 March 2011
r84277Follow-up r83658 CR: Undoing addition of second argument, prefix is for ident...krinkle22:22, 18 March 2011

Comments

#Comment by Mdale (talk | contribs)   00:57, 12 March 2011

The "prefix" is not meant to be override by calling. Its to be used as a global config for when you have multiple iframes interacting and its very difficult to trace which iframe is doing what ( not a way to concatenate the method ;) You set it globally for the given iframe instance.

You could potentially get at the call method via a getStackTrace() call ( bug 27999 ) .. but that its slow and does not show the parent object. IMO its best to just do normal string concatenation within the function doing the logging.

#Comment by Krinkle (talk | contribs)   23:28, 18 March 2011

Some documentation on this wouldn't hurt. Can you show some example uses ?

It doesn't seem to be used anywhere right now (either in core or in extensions) ("mw.log.prefix" in /trunk

bytheway: That part of this commit was reverted by r84277 and other follow-ups. Setting status to 'new'.

#Comment by Mdale (talk | contribs)   23:48, 18 March 2011

Yes, documentation would be nice.

Example use-cases include the iframe api for the html5 player: http://svn.wikimedia.org/svnroot/mediawiki/trunk/extensions/TimedMediaHandler/MwEmbedModules/EmbedPlayer/resources/iframeApi/

and the api proxy used for cross site uploading. Old version: http://svn.wikimedia.org/svnroot/mediawiki/branches/MwEmbedStandAlone/modules/ApiProxy/mw.ApiProxy.js

The api proxy will be rewritten to use html5 postMessage ( like the html5 player api )

at any rate its useful in these cases as long as we keep the prefix as a global.

Status & tagging log