"Krinkle" posted a comment on MediaWiki.r106706.
URL: http://www.mediawiki.org/wiki/Special:Code/MediaWiki/106706#c28088
Commit summary for MediaWiki.r106706:
[WebFonts] Code quality & clean up
* Various good practices, white space conventions (Thanks JSLint)
* Such as:
-- JavaScript does not have block scope! Removed var statements from loops, and
put them outside the loop for clarity
-- Optimize a few loops (swapped a 'for in' loop for a 'for i < length' loop,
since it was an array, and improved the existing one by caching the length
property since otherwise the length property would be accessed every iteration
- also prevents infinite loops in some situations)
-- Combining var statements. Ideally every function only needs 1 var statement
(since there is only function scope and everything is hoisted internally
anyway), but no time to go through and restructure them all right now, did most
of them.
-- Improve function documentation ('@param config The webfont configuration.'
-> '@param fonts {Array} List of fonts to be provided as a menu option.')
-- Removed unused variables (such as 'languages' in
loadFontsForFontFamilyStyle, added in r100677)
-- Using strict comparison to false values (match(..) === null), both for speed
gain and improve readability as loose comparison is ambiguous in JavaScript
(val == null also returns true for undefined instead of null)
-- Using direct property access and strict comparison instead of an ' in '
lookup
-- Rename a variable called 'config' to 'fonts' (array) to avoid confusion with
another variable that is a plain object also called 'config'
-- Make 'mw' a local variable by passing it through the IIFE closure from the
global scope (for faster lookups in the context object)
-- Using the "||" operator in a variable assignment as "var = foo || bar"
instead of "var = foo ? foo : bar". In JavaScript comparison operators do not
return boolean, instead they return the last compared value. (which in an
if-statement is then type-coerced to a boolean)
-- Using mw.util.addCSS instead of creating a new <style> element locally.
-- Trailing whitespace
-- Redundant spaces (e.g. double space where single space was intended)
Krinkle's comment:
Interesting, I didn't know that. But that StackOverflow thread is about a bug
in the <code>addrule</code> method of Internet Explorer's
<code>styleSheet</code> object representing a stylesheet that is currently
loaded in the document.
<code>[[RL/DM#addCSS|mw.util.addCSS]]</code> doesn't use this method due and
isn't modifying existing stylesheets. It adds new stylesheets exactly the way
that StackOverflow thread suggests.
Although I didn't know about this particular bug in IE, it was due to the
cross-browser differences in this object that I choose to insert new
stylesheets (which is slower) instead of touching existing ones.
<code>mw.util.addCSS</code> can be used here perfectly fine as all it does is
exactly what you want. It inserts an extra {{tag|style|o}} element. See also
the
[//svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/resources/mediawiki/mediawiki.util.js?revision=96151&view=markup#l146
source of mw.util.addCSS] and [[RL/DM#addCSS|documentation]].
I'm not going to revert anything as I'm keeping the possibility open something
may be broken in <code>mw.util</cde> indeed. Did you verify that using
<code>mw.util.addCSS</code> does not work here for IE ?
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
