MediaWiki r110542 - Code Review

Jump to: navigation, search
Repository:MediaWiki
Revision:< r110541‎ | r110542 (on ViewVC)‎ | r110543 >
Date:00:47, 2 February 2012
Author:krinkle
Status:resolved (Comments)
Tags:core 
Comment:
Enqueue mw.util.init in document-ready even handler earlier
* Previously $(document).ready(mw.util.init) was in module 'mediawiki.page.ready' (position: bottom). I've now moved this to 'mediawiki.page.startup' so that it'll be enqueued sooner.
* This making it more likely that if someone also enqueues in document-ready that mw.util.init ran before than and thus mw.util.$content populated
* Fixes bug 33711

* All this is still depends on the order in which the event handler queue is executed, which is risky. Bug 30713 will bring the solid "watertight" solution
Modified paths:

Diff [purge]

Index: trunk/phase3/resources/Resources.php
——@@ -726,6 +726,7 @@
727727 'scripts' => 'resources/mediawiki.page/mediawiki.page.startup.js',
728728 'dependencies' => array(
729729 'jquery.client',
 730+ 'mediawiki.util',
730731 ),
731732 'position' => 'top',
732733 ),
Index: trunk/phase3/resources/mediawiki.page/mediawiki.page.startup.js
——@@ -1,4 +1,4 @@
2 -( function( $ ) {
 2+( function ( $ ) {
33
44 mw.page = {};
55
——@@ -8,4 +8,11 @@
99 .addClass('client-js' )
1010 .removeClass( 'client-nojs' );
1111
 12+ // Initialize utilities as soon as the document is ready (mw.util.$content,
 13+ // messageBoxNew, profile, tooltip access keys, Table of contents toggle, ..).
 14+ // Enqueued into domready from here instead of mediawiki.page.ready to ensure that it gets enqueued
 15+ // before other modules hook into document ready, so that mw.util.$content (defined by mw.util.init),
 16+ // is defined for them.
 17+ $( mw.util.init );
 18+
1219 } )( jQuery );
Index: trunk/phase3/resources/mediawiki.page/mediawiki.page.ready.js
——@@ -1,8 +1,5 @@
22 jQuery( document ).ready( function( $ ) {
33
4 - /* Initialize utilities (messageBoxNew, profile, tooltip access keys, TOC etc.) */
5 - mw.util.init();
6 -
74 /* Emulate placeholder if not supported by browser */
85 if ( !( 'placeholder' in document.createElement( 'input' ) ) ) {
96 $( 'input[placeholder]' ).placeholder();

Follow-up revisions

Rev.Commit summaryAuthorDate
r110619Followup r110542: unbreak the QUnit tests; the change wasn't broken but the t...catrope21:41, 2 February 2012

Comments

#Comment by Siebrand (talk | contribs)   21:34, 2 February 2012

This killed unit tests. http://integration.mediawiki.org/testswarm/job/1091/

#Comment by Krinkle (talk | contribs)   21:50, 2 February 2012
Followup r110542: unbreak the QUnit tests; the change wasn't broken but the test runner was. Reflect the dependency change (mw.page.startup now depends on mw.util) by moving up mw.util . It's kind of annoying that the test suite doesn't use the dependency map from Resources.php

Aha, spot on. I'm actually using Special:JavaScriptTest/qunit locally instead of the old qunit/tests/index.html. We need to get that tested on labs asap and deployed on live integration server.

Status & tagging log

  • 10:55, 14 April 2012 Krinkle (talk | contribs) changed the tags for r110542 [removed: frontend]
  • 21:41, 2 February 2012 Catrope (talk | contribs) changed the status of r110542 [removed: fixme added: resolved]
  • 21:34, 2 February 2012 Siebrand (talk | contribs) changed the status of r110542 [removed: ok added: fixme]
  • 20:53, 2 February 2012 Catrope (talk | contribs) changed the status of r110542 [removed: new added: ok]
  • 03:09, 2 February 2012 MarkAHershberger (talk | contribs) changed the tags for r110542 [added: core,frontend]
Retrieved from "http://www.mediawiki.org/wiki/Special:Code/MediaWiki/110542"