r73499 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r73498‎ | r73499 | r73500 >
Date:00:05, 22 September 2010
Author:tparscal
Status:ok (Comments)
Tags:
Comment:
Removed unneeded mediaWiki.loader.using wrapper which got the old toolbar working again. Fixes bug #25239.
Modified paths:
  • /trunk/phase3/includes/EditPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/EditPage.php
@@ -2246,7 +2246,7 @@
22472247 }
22482248
22492249 $wgOut->addScript( Html::inlineScript(
2250 - "if ( mediaWiki !== undefined ) { mediaWiki.loader.using( 'mediawiki.legacy.edit', function() { $script } ); }"
 2250+ "if ( mediaWiki !== undefined ) { $script }"
22512251 ) );
22522252
22532253 $toolbar .= "\n</div>";

Follow-up revisions

RevisionCommit summaryAuthorDate
r73550Fixed check for mediaWiki existance and removed additional unneeded wrapping ...tparscal18:03, 22 September 2010
r73618Fixed typo pointed out by Icefox - thanks! See comments for r73499.tparscal18:08, 23 September 2010

Comments

#Comment by Icefox~mediawikiwiki (talk | contribs)   15:34, 22 September 2010

mediawiki.js contains the lines (In mediaWiki.loader.using):

				if ( typeof ready !== 'function' ) {
					ready();
				}

That's why the callback is never called.

On a side note, the global constant undefined is not defined in IE 5.0 and earlier, so you might want to replace

"if ( mediaWiki !== undefined )

with

"if ( window.mediaWiki )

to avoid js errors in old browser.

#Comment by Catrope (talk | contribs)   16:15, 22 September 2010

Surely typeof mediaWiki !== 'undefined' will work?

#Comment by Icefox~mediawikiwiki (talk | contribs)   17:15, 22 September 2010

Yes, that'll work in all browsers I know of, though it's IMHO unnecessarily long and also somewhat slower than what I suggested :)

#Comment by Simetrical (talk | contribs)   17:17, 22 September 2010

Our JS is probably thoroughly broken in IE5 anyway, so I'd ignore it for this purpose. IE6 and up is enough of a pain to care about.

#Comment by Catrope (talk | contribs)   17:50, 22 September 2010

We have a bootstrapper whose sole responsibility is to not load jQuery, mediaWiki and any other JS in IE < 6 (other browsers will be added soon as well), so at least we won't get JS errors in IE5.

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:05, 22 September 2010

I fixed this up a bit. As for ignoring old browser - that's exactly what this code is for, old browsers, so it needs to work on them.

#Comment by Simetrical (talk | contribs)   19:29, 22 September 2010

Hmm, okay. Clearly I should read more carefully before commenting.  :) If anyone's making sure basic functionality works in IE5, that's fine by me (although if no one is, that's fine by me too).

#Comment by Icefox~mediawikiwiki (talk | contribs)   17:35, 23 September 2010

Thanks, but please note that the typo I mentioned in my first comment is still there. To make it more clear the line

				if ( typeof ready !== 'function' ) {

from mediawiki.js, function mediaWiki.loader.using should be replaced with

				if ( typeof ready === 'function' ) {

Cheers

#Comment by Trevor Parscal (WMF) (talk | contribs)   18:09, 23 September 2010

Sorry, I keep meaning to get this fixed only to get distracted. It's fixed now in r73618.

Status & tagging log