r55569 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r55568‎ | r55569 | r55570 >
Date:13:56, 25 August 2009
Author:nikerabbit
Status:ok (Comments)
Tags:
Comment:
* Revert r55555-55557 for now, breaks tabs in special preferences, don't know why
Also, the following looks wrong:
- for (var id = 0; id < ta.length; id++) {
+ for (var id in ta) {
var n = document.getElementById(id);
Modified paths:
  • /trunk/phase3/skins/common/mwsuggest.js (modified) (history)
  • /trunk/phase3/skins/common/protect.js (modified) (history)
  • /trunk/phase3/skins/common/wikibits.js (modified) (history)

Diff [purge]

Index: trunk/phase3/skins/common/protect.js
——@@ -250,7 +250,7 @@
251251 */
252252 'getLevelSelectors': function() {
253253 var all = document.getElementsByTagName("select");
254 - var ours = [];
 254+ var ours = new Array();
255255 for (var i = 0; i < all.length; i++) {
256256 var element = all[i];
257257 if (element.id.match(/^mwProtect-level-/)) {
——@@ -279,7 +279,7 @@
280280 */
281281 'getExpiryInputs': function() {
282282 var all = document.getElementsByTagName("input");
283 - var ours = [];
 283+ var ours = new Array();
284284 for (var i = 0; i < all.length; i++) {
285285 var element = all[i];
286286 if (element.name.match(/^mwProtect-expiry-/)) {
——@@ -307,7 +307,7 @@
308308 */
309309 'getExpirySelectors': function() {
310310 var all = document.getElementsByTagName("select");
311 - var ours = [];
 311+ var ours = new Array();
312312 for (var i = 0; i < all.length; i++) {
313313 var element = all[i];
314314 if (element.id.match(/^mwProtectExpirySelection-/)) {
Index: trunk/phase3/skins/common/mwsuggest.js
——@@ -24,8 +24,8 @@
2525 // delay between keypress and suggestion (in ms)
2626 var os_search_timeout = 250;
2727 // these pairs of inputs/forms will be autoloaded at startup
28 -var os_autoload_inputs = ['searchInput', 'searchInput2', 'powerSearchText', 'searchText'];
29 -var os_autoload_forms = ['searchform', 'searchform2', 'powersearch', 'search'];
 28+var os_autoload_inputs = new Array('searchInput', 'searchInput2', 'powerSearchText', 'searchText');
 29+var os_autoload_forms = new Array('searchform', 'searchform2', 'powersearch', 'search' );
3030 // if we stopped the service
3131 var os_is_stopped = false;
3232 // max lines to show in suggest table
——@@ -364,7 +364,7 @@
365365 var c = document.getElementById(r.container);
366366 var width = c.offsetWidth - os_operaWidthFix(c.offsetWidth);
367367 var html = "<table class=\"os-suggest-results\" id=\""+r.resultTable+"\" style=\"width: "+width+"px;\">";
368 - r.results = [];
 368+ r.results = new Array();
369369 r.resultCount = results.length;
370370 for(i=0;i<results.length;i++){
371371 var title = os_decodeValue(results[i]);
Index: trunk/phase3/skins/common/wikibits.js
——@@ -181,14 +181,23 @@
182182 var mwCustomEditButtons = []; // eg to add in MediaWiki:Common.js
183183
184184 function escapeQuotes(text) {
185 - return escapeQuotesHTML(text.replace(/'/g,"\\'").replace(/\n/g,"\\n"));
 185+ var re = new RegExp("'","g");
 186+ text = text.replace(re,"\\'");
 187+ re = new RegExp("\\n","g");
 188+ text = text.replace(re,"\\n");
 189+ return escapeQuotesHTML(text);
186190 }
187191
188192 function escapeQuotesHTML(text) {
189 - return text.replace(/&/g,"&amp;")
190 - .replace(/"/g,"&quot;")
191 - .replace(/</gre,"&lt;")
192 - .replace(/>/g,"&gt;");
 193+ var re = new RegExp('&',"g");
 194+ text = text.replace(re,"&amp;");
 195+ re = new RegExp('"',"g");
 196+ text = text.replace(re,"&quot;");
 197+ re = new RegExp('<',"g");
 198+ text = text.replace(re,"&lt;");
 199+ re = new RegExp('>',"g");
 200+ text = text.replace(re,"&gt;");
 201+ return text;
193202 }
194203
195204
——@@ -299,7 +308,7 @@
300309 link.setAttribute( "title", tooltip );
301310 }
302311 if ( accesskey && tooltip ) {
303 - updateTooltipAccessKeys( [link] );
 312+ updateTooltipAccessKeys( new Array( link ) );
304313 }
305314
306315 if ( nextnode && nextnode.parentNode == node )
——@@ -345,7 +354,7 @@
346355 // A lot of user scripts (and some of the code below) break if
347356 // ta isn't defined, so we make sure it is. Explictly using
348357 // window.ta avoids a "ta is not defined" error.
349 - if (!window.ta) window.ta = [];
 358+ if (!window.ta) window.ta = new Array;
350359
351360 // Make a local, possibly restricted, copy to avoid clobbering
352361 // the original.
——@@ -358,7 +367,7 @@
359368
360369 // Now deal with evil deprecated ta
361370 var watchCheckboxExists = document.getElementById( 'wpWatchthis' ) ? true : false;
362 - for (var id = 0; id < ta.length; id++) {
 371+ for (var id in ta) {
363372 var n = document.getElementById(id);
364373 if (n) {
365374 var a = null;
——@@ -489,7 +498,7 @@
490499 From http://www.robertnyman.com/2005/11/07/the-ultimate-getelementsbyclassname/
491500 */
492501 function getElementsByClassName(oElm, strTagName, oClassNames){
493 - var arrReturnElements = [];
 502+ var arrReturnElements = new Array();
494503 if ( typeof( oElm.getElementsByClassName ) == "function" ) {
495504 /* Use a native implementation where possible FF3, Saf3.2, Opera 9.5 */
496505 var arrNativeReturn = oElm.getElementsByClassName( oClassNames );
——@@ -502,7 +511,7 @@
503512 return arrReturnElements;
504513 }
505514 var arrElements = (strTagName == "*" && oElm.all)? oElm.all : oElm.getElementsByTagName(strTagName);
506 - var arrRegExpClassNames = [];
 515+ var arrRegExpClassNames = new Array();
507516 if(typeof oClassNames == "object"){
508517 for(var i=0; i<oClassNames.length; i++){
509518 arrRegExpClassNames[arrRegExpClassNames.length] =
——@@ -671,8 +680,8 @@
672681
673682 var reverse = (span.getAttribute("sortdir") == 'down');
674683
675 - var newRows = [];
676 - var staticRows = [];
 684+ var newRows = new Array();
 685+ var staticRows = new Array();
677686 for (var j = rowStart; j < table.rows.length; j++) {
678687 var row = table.rows[j];
679688 if((" "+row.className+" ").indexOf(" unsortable ") < 0) {
——@@ -680,8 +689,8 @@
681690 var oldIndex = (reverse ? -j : j);
682691 var preprocessed = preprocessor( keyText.replace(/^[\s\xa0]+/, "").replace(/[\s\xa0]+$/, "") );
683692
684 - newRows[newRows.length] = [row, preprocessed, oldIndex];
685 - } else staticRows[staticRows.length] = [row, false, j-rowStart];
 693+ newRows[newRows.length] = new Array(row, preprocessed, oldIndex);
 694+ } else staticRows[staticRows.length] = new Array(row, false, j-rowStart);
686695 }
687696
688697 newRows.sort(sortfn);

Past revisions this follows-up on

RevisionCommit summaryAuthorDate
r55555- Use array literals instead of new Array(); (note that `new Array;` without ...dantman17:15, 24 August 2009

Comments

#Comment by Brion VIBBER (talk | contribs)   17:01, 26 August 2009

The most obvious bug I see from those previous optimizations was a stray "re" that ended up smooshed into a regex literal, breaking everything. Otherwise they look ok, I think...

Note that we've got several more of the 'for (var i in someArray)' pattern, which is problematic if something like the Prototype library gets loaded, as it adds extra methods to the Array type which ends up interfering with that loop pattern; they should all be changed to use explicit indexes. (That's an existing issue and already noted in Bugzilla.)

Status & tagging log