r111989 MediaWiki - Code Review archive

I haven't tested any of this, but I see a few pieces of concern that you may not have been aware of. Nothing critical, just small bits and pieces.

- var $filterBox = $( '#' + mw.config.get( 'abuseFilterBoxName' ) );
+ var $filterBox = [];
..
- var $syntaxResult = $( '#mw-abusefilter-syntaxresult' );
+ var $syntaxResult = [];

This file contains several references to $filterBox outside the document-ready callback. I don't know if those methods are called directly but it feels wrong. But initializing it to an empty array doesn't make sense either way. Things like $filterBox.val() will fail if it's not a jQuery object. Either leave it undefined (var $filterBox;) or initialize it to an empty jQuery object (var $filterBox = $( [] );) to keep it compatible in a lazy way.

- (function($,mw) {
-    $("div.searchresults")
- })( jQuery, window.mediaWiki );
+ jQuery( document ).ready( function() {
+   jQuery("div.searchresults")
+ } );

$ can be aliased locally without an additional closure via the first argument of the document-ready callback.

+ jQuery( document ).ready( function() {
    var wgDonateButton = getDonateButton();
..
+ } );

Again, I don't know this extension. But this can potentially break stuff if anything else outside this file was referring to wgDonateButton, which is now in a "lower" context, and no longer exposed. It wasn't a global since ResourceLoader uses closures per module, but it was exposed to other files within the same module (if the module has multiple files). Probably not an issue here thoguh (unless this file is not loaded with ResourceLoader in which case the breakage much more likely).

Initialising to an empty array was Roan's idea, he told me to do it when I wrote r111983. There were some instances in the core already of the same pattern. You're right that it seems a bit wrong.

I don't see the point in creating a $ alias for a single-line function.

Yes there are references $filterBox, but only in functions called from callbacks registered in the document ready hook. Note that the object is anonymous and does not provide any global variables which would allow it to be called from an external module or from HTML, so the ready callback is the only entry point.

The wgDonateButton thing does appear to be a bug.

Alrighty.

by the way, the alias. I meant something like this:

jQuery( document ).ready( function( $ ) {
   $("div.searchresults")
} );

saves a few chars and slight speed improvement by accessing jQuery via $ from the local scope instead of several steps up in the chain to the window property.

For the record, on my laptop with Firefox 10, a global variable reference takes about 0.6us longer than a local variable reference, and running jQuery('div.searchresults').find('a[href*="action=edit"]') on a typical search results page takes 250us.

For the record, I did say to initialize to $([]), not [].