r90359 MediaWiki - Code Review archive

This breaks Javascript tests: http://toolserver.org/~krinkle/testswarm/job/123/

Marking fixme :)

jquery.tabindex.js under Opera 11.0x:

http://toolserver.org/~krinkle/testswarm/?state=runresults&run_id=718&client_id=1824

mediawiki.Title.js under Opera 11.1x:

http://toolserver.org/~krinkle/testswarm/?state=runresults&run_id=719&client_id=1822

See Manual:JavaScript unit testing for help about running the test yourself :)

Oh, sorry. I can't reproduce this on Windows, though. Nor have I any idea what is causing it or how to fix it.

When in doubt, back it out :)

Those tests don't look like they could be affected by this change in any way, as no file changed by this commit is loaded in the tests.

Removing fixme.

Can't repro errors on Opera 11.11/OS X either -- probably temporary problems from files not getting reloaded properly or something.

Thanks brion for testing this. I will poke Krinkle about this to check if it might be a bug in testswarm.

Hi folks,

Just got an e-mail from Hashar. Looks like the Opera-client that was connected to the swarm had a short connection interruption half-way loading the page. This caused a few scripts to occasionally not be loaded. Not much TestSwarm can do about this.

TestSwarm has a setting for each "project" which controls how "certain" TestSwarm must be before marking a test as broken and continuing with the next test. For MediaWiki this setting is set to 3, this means if a client (say a browser with the Presto 2.7 engine) reports a test to be broken, TestSwarm will, if and when a similar browser is available again, re-send the test up to three times in attempt to get a "success". Only after three times it will continue to the next test job for that browser.

The reason the MediaWiki-project page kept showing the test as broken was likely because the Opera-client disconnected right after running that 1 test. I've reconnected two Opera-clients locally here and as expected they were first fed the broken tests and they are now showing up as succeeded without errors :-)

http://toolserver.org/~krinkle/testswarm/user/MediaWiki

As you can see the Opera 11.1 turned green. I don't have Opera 11.0 here though, if you do, simply reconnect it and it should fix itself.

Aside from that, marking as fixme per the next few points:

• Why is this callback function returning false ? It is therewith blocking any and all other events that may be bound, use e.preventDefault() instead.
• Although the difference in this particular case is quite small (since SpecialUndelete pages are usually short), the selector is ineffecient right now. It uses "input:checkbox" which means all ‎<input> elements on the page are queried and one-by-one checked to see if it matches the checkbox-profile. On my localhost a gadget puts a checkbox next to the searchbar in Vector, this is wrongly toggled when I click the Invert button now! Instead make use of the context.

$('#mw-undelete-table') /* <= seperate find() call to speed up the ID-selector */.find('input:checkbox') /* <= only looks within the undelete-table */

• Right now there are four anonymous functions. A closure to alias $ to jQuery, a shortcut to $(document).ready with $(fn), a function passed to click(), and another one passed to .each(). The former two can be merged by using the fact that the document-ready function is called with jQuery as first argument. This saves an additional function call and a little bandwidth as well. The latter each() is redundant as jQuery's internal methods automatically do each (ie. $('.foo').attr('title', 'Hello') sets the attribute for each of the selected elements, no need for an each() construction. The internal looping system is a light-weight for()-loop that optionally takes a function for non-static inputs, such as "!value" like is the case here.)
• Attribution is done through Wikimedia-specific interwiki links. Aside from them obviously not working in a .js file (remember, this is not a Gadget), they are also Wikimedia-specific, please use full URLs where possible. Due to the simplicity of this function though and the convention of not listing authors for individual functions, perhaps it should be removed.
• Check out the whitespace conventions there should be more spaces in function calls etc.

<source lang="javascript"> jQuery( document ).ready( function( $ ) {

 $( '#mw-undelete-invert' ).click( function( e ) {
   e.preventDefault();
   $( '#mw-undelete-table' ).find( 'input:checkbox' ).prop( 'checked', function( i, val ) { return !val; } );
 });

} );

Firstly, I'm deeply sorry for committing the code so headlessly and thanks for all those valuable hints.

Additionally, regarding the second point: what is the difference between $('#mw-undelete-table input:checkbox') and $('#mw-undelete-table').find('input:checkbox') and how significant is the performance difference?

Thank you for these explanations.

I wrote down a few words regarding this at JSPERF. Check out Selector performance.