r94332 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r94331‎ | r94332 | r94333 >
Date:09:27, 12 August 2011
Author:dantman
Status:ok (Comments)
Tags:
Comment:
Fix usage of the jQuery global in a few spots.
- jQuery changed to $ in some files because there is a closure that creates a locally scoped $, but the jQuery var is globally scoped, meaning using jQuery instead of $ inside that closure could result in interacting with a different instance of jQuery than the uses of $ in that same closure.
- In mwExtension wrap the code inside a closure which it is missing. Also take this chance to fix the whitespace style `fn( arg )` instead of `fn(arg)` on the isArray I added.
This is partially a followup to r94331.

Note: The jquery plugins inside the jquery/ folder look fine for use of jQuery within closures, except for mockjax.
Modified paths:
  • /trunk/phase3/resources/jquery/jquery.makeCollapsible.js (modified) (history)
  • /trunk/phase3/resources/jquery/jquery.mwExtension.js (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.js (modified) (history)
  • /trunk/phase3/resources/mediawiki/mediawiki.util.js (modified) (history)

Diff [purge]

Index: trunk/phase3/resources/jquery/jquery.makeCollapsible.js
——@@ -38,7 +38,7 @@
3939 if ( typeof $defaultToggle == 'undefined' ) {
4040 $defaultToggle = null;
4141 }
42 - if ( $defaultToggle !== null && !($defaultToggle instanceof jQuery) ) {
 42+ if ( $defaultToggle !== null && !($defaultToggle instanceof $) ) {
4343 // is optional (may be undefined), but if defined it must be an instance of jQuery.
4444 // If it's not, abort right away.
4545 // After this $defaultToggle is either null or a valid jQuery instance.
——@@ -336,4 +336,4 @@
337337 }
338338 } );
339339 };
340 -} )( jQuery, mediaWiki );
\ No newline at end of file
 340+} )( jQuery, mediaWiki );
Index: trunk/phase3/resources/jquery/jquery.mwExtension.js
——@@ -1,119 +1,121 @@
22 /*
33 * JavaScript backwards-compatibility alternatives and other convenience functions
44 */
 5+( function( $ ) {
56
6 -jQuery.extend({
7 - trimLeft: function( str ) {
8 - return str === null ? '' : str.toString().replace( /^\s+/, '' );
9 - },
10 - trimRight: function( str ) {
11 - return str === null ?
12 - '' : str.toString().replace( /\s+$/, '' );
13 - },
14 - ucFirst: function( str ) {
15 - return str.substr( 0, 1 ).toUpperCase() + str.substr( 1 );
16 - },
17 - escapeRE: function( str ) {
18 - return str.replace ( /([\\{}()|.?*+\-^$\[\]])/g, "\\$1" );
19 - },
20 - isDomElement: function( el ) {
21 - return !!el && !!el.nodeType;
22 - },
23 - isEmpty: function( v ) {
24 - if ( v === '' || v === 0 || v === '0' || v === null
25 - || v === false || v === undefined )
26 - {
27 - return true;
28 - }
29 - // the for-loop could potentially contain prototypes
30 - // to avoid that we check it's length first
31 - if ( v.length === 0 ) {
32 - return true;
33 - }
34 - if ( typeof v === 'object' ) {
35 - for ( var key in v ) {
36 - return false;
 7+ $.extend({
 8+ trimLeft: function( str ) {
 9+ return str === null ? '' : str.toString().replace( /^\s+/, '' );
 10+ },
 11+ trimRight: function( str ) {
 12+ return str === null ?
 13+ '' : str.toString().replace( /\s+$/, '' );
 14+ },
 15+ ucFirst: function( str ) {
 16+ return str.substr( 0, 1 ).toUpperCase() + str.substr( 1 );
 17+ },
 18+ escapeRE: function( str ) {
 19+ return str.replace ( /([\\{}()|.?*+\-^$\[\]])/g, "\\$1" );
 20+ },
 21+ isDomElement: function( el ) {
 22+ return !!el && !!el.nodeType;
 23+ },
 24+ isEmpty: function( v ) {
 25+ if ( v === '' || v === 0 || v === '0' || v === null
 26+ || v === false || v === undefined )
 27+ {
 28+ return true;
3729 }
38 - return true;
39 - }
40 - return false;
41 - },
42 - compareArray: function( arrThis, arrAgainst ) {
43 - if ( arrThis.length != arrAgainst.length ) {
44 - return false;
45 - }
46 - for ( var i = 0; i < arrThis.length; i++ ) {
47 - if ( $.isArray(arrThis[i]) ) {
48 - if ( !$.compareArray( arrThis[i], arrAgainst[i] ) ) {
 30+ // the for-loop could potentially contain prototypes
 31+ // to avoid that we check it's length first
 32+ if ( v.length === 0 ) {
 33+ return true;
 34+ }
 35+ if ( typeof v === 'object' ) {
 36+ for ( var key in v ) {
4937 return false;
5038 }
51 - } else if ( arrThis[i] !== arrAgainst[i] ) {
 39+ return true;
 40+ }
 41+ return false;
 42+ },
 43+ compareArray: function( arrThis, arrAgainst ) {
 44+ if ( arrThis.length != arrAgainst.length ) {
5245 return false;
5346 }
54 - }
55 - return true;
56 - },
57 - compareObject: function( objectA, objectB ) {
 47+ for ( var i = 0; i < arrThis.length; i++ ) {
 48+ if ( $.isArray( arrThis[i] ) ) {
 49+ if ( !$.compareArray( arrThis[i], arrAgainst[i] ) ) {
 50+ return false;
 51+ }
 52+ } else if ( arrThis[i] !== arrAgainst[i] ) {
 53+ return false;
 54+ }
 55+ }
 56+ return true;
 57+ },
 58+ compareObject: function( objectA, objectB ) {
5859
59 - // Do a simple check if the types match
60 - if ( typeof objectA == typeof objectB ) {
 60+ // Do a simple check if the types match
 61+ if ( typeof objectA == typeof objectB ) {
6162
62 - // Only loop over the contents if it really is an object
63 - if ( typeof objectA == 'object' ) {
64 - // If they are aliases of the same object (ie. mw and mediaWiki) return now
65 - if ( objectA === objectB ) {
66 - return true;
67 - } else {
68 - var prop;
69 - // Iterate over each property
70 - for ( prop in objectA ) {
71 - // Check if this property is also present in the other object
72 - if ( prop in objectB ) {
73 - // Compare the types of the properties
74 - var type = typeof objectA[prop];
75 - if ( type == typeof objectB[prop] ) {
76 - // Recursively check objects inside this one
77 - switch ( type ) {
78 - case 'object' :
79 - if ( !$.compareObject( objectA[prop], objectB[prop] ) ) {
80 - return false;
81 - }
82 - break;
83 - case 'function' :
84 - // Functions need to be strings to compare them properly
85 - if ( objectA[prop].toString() !== objectB[prop].toString() ) {
86 - return false;
87 - }
88 - break;
89 - default:
90 - // Strings, numbers
91 - if ( objectA[prop] !== objectB[prop] ) {
92 - return false;
93 - }
94 - break;
 63+ // Only loop over the contents if it really is an object
 64+ if ( typeof objectA == 'object' ) {
 65+ // If they are aliases of the same object (ie. mw and mediaWiki) return now
 66+ if ( objectA === objectB ) {
 67+ return true;
 68+ } else {
 69+ var prop;
 70+ // Iterate over each property
 71+ for ( prop in objectA ) {
 72+ // Check if this property is also present in the other object
 73+ if ( prop in objectB ) {
 74+ // Compare the types of the properties
 75+ var type = typeof objectA[prop];
 76+ if ( type == typeof objectB[prop] ) {
 77+ // Recursively check objects inside this one
 78+ switch ( type ) {
 79+ case 'object' :
 80+ if ( !$.compareObject( objectA[prop], objectB[prop] ) ) {
 81+ return false;
 82+ }
 83+ break;
 84+ case 'function' :
 85+ // Functions need to be strings to compare them properly
 86+ if ( objectA[prop].toString() !== objectB[prop].toString() ) {
 87+ return false;
 88+ }
 89+ break;
 90+ default:
 91+ // Strings, numbers
 92+ if ( objectA[prop] !== objectB[prop] ) {
 93+ return false;
 94+ }
 95+ break;
 96+ }
 97+ } else {
 98+ return false;
9599 }
96100 } else {
97101 return false;
98102 }
99 - } else {
100 - return false;
101103 }
102 - }
103 - // Check for properties in B but not in A
104 - // This is about 15% faster (tested in Safari 5 and Firefox 3.6)
105 - // ...than incrementing a count variable in the above and below loops
106 - // See also: http://www.mediawiki.org/wiki/ResourceLoader/Default_modules/compareObject_test#Results
107 - for ( prop in objectB ) {
108 - if ( !( prop in objectA ) ) {
109 - return false;
 104+ // Check for properties in B but not in A
 105+ // This is about 15% faster (tested in Safari 5 and Firefox 3.6)
 106+ // ...than incrementing a count variable in the above and below loops
 107+ // See also: http://www.mediawiki.org/wiki/ResourceLoader/Default_modules/compareObject_test#Results
 108+ for ( prop in objectB ) {
 109+ if ( !( prop in objectA ) ) {
 110+ return false;
 111+ }
110112 }
111113 }
112114 }
 115+ } else {
 116+ return false;
113117 }
114 - } else {
115 - return false;
 118+ return true;
116119 }
117 - return true;
118 - }
119 -});
 120+ });
120121
 122+} )( jQuery );
Index: trunk/phase3/resources/mediawiki/mediawiki.util.js
——@@ -267,7 +267,7 @@
268268
269269 return;
270270
271 - } else if ( nodeList instanceof jQuery ) {
 271+ } else if ( nodeList instanceof $ ) {
272272 $nodes = nodeList;
273273 } else {
274274 $nodes = $( nodeList );
Index: trunk/phase3/resources/mediawiki/mediawiki.js
——@@ -558,7 +558,7 @@
559559 } );
560560 }
561561 } else if ( $.isFunction( script ) ) {
562 - script( jQuery );
 562+ script( $ );
563563 markModuleReady();
564564 }
565565 } catch ( e ) {

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r94331Use jQuery's $.isArray, not instanceof Array. The later has troubles with cro...dantman08:40, 12 August 2011

Comments

#Comment by Krinkle (talk | contribs)   09:34, 12 August 2011

Indention in jquery.mwExtension.js is bit nasty in review but viewvc has the whitespace flag on by default

http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/resources/jquery/jquery.mwExtension.js?&pathrev=94332&r1=94331&r2=94332 (yeah, that took a bunch of ‎<nowiki>'s to not make CodeReview choke on it.

Looks good, marking ok.

#Comment by Krinkle (talk | contribs)   09:35, 12 August 2011

We may wanna poke mockjax upstream though.

Status & tagging log