User:Krinkle/Extension review/WebFonts

Done: Globals
Usage of config globals in JavaScript has been deprecated as of 1.18. It still works by default but wikis can disable it at will. In which case the following error appears and the rest is not loaded:

Use  instead.

Review r102632: HTML fragments
This fails in older versions of Internet Explorer. jQuery creates it's fragments using innerHTML. As documented, shorthand for fragments is not allowed. Input must be valid HTML, since a self-closing anchor tag is invalid, this fails in IE which is more stricter than other browsers.

For the creation of elements (instead of fragments), self-closing shorthand is allowed but there is no need to do so. And the alternative is shorter as well. (  instead of   ).

Done: Dependencies
I see several uses of $.cookie, don't forget to add the  module to the dependencies of your module to make sure it's loaded correctly.

Done: User options / wgWebFontsEnabledByDefault

 * See also

Since it's used in PHP as a preference, please use it as a preference here as well. This condition doesn't make sense as this ignores logged-in users. Use  instead. Does that mean wgWebFontsEnabledByDefault no longer has to be exported to JavaScript ?

Alternatively you may want to remove the condition all together since the module is only loaded by PHP if the user has this preference anyway:

Done: Logging
Use  instead of conditionally calling. It is a no-op in production mode and in debug mode it either reroutes to console.log or creates a html-console on the fly.

Done: jQuery stuff
You can use multiple selectors separated by comma, just like in CSS. No need to repeat it twice.

I see this in another part of the code as well. Use jQuery method "chaining". Like:

You probably mean the -property instead of the  -attribute. Use  instead, as changing the attribute will not change the state of the checkbox (browsers use the attribute value to determine the state of the checkbox when the page loads, after that only the property is used and the attribute is ignored).

In older versions of jQuery one had to use, later they builded a mapping system into   to automatically update the property as well, however since it's not really an attribute they created   in jQuery 1.6, please use it.

Read more about the difference between attributes and properties here. See also .prop and .attr

Please be consistent. I'd recommend using, since that's what the code actually returns (it returns the index or  ).

Done: Code conventions
Nothing major, just a few minor points. See also the code conventions page for a more complete list, not going to repeat them here.

Done: Default user options
This one is somewhat confusing. The confusion is caused because the default value for that is already set to true from the main WebFonts.php file. The  setting is now ignored for logged-in users, because it's set to true no matter what. The solution is fairly simple though, simply set the default user option value to " ". However do so from the UserGetDefaultOptions hook (since setting it from the main php file would leave no way for a wiki to actually configure wgWebFontsEnabledByDefault from LocalSettings.php).

Done: File structure
General structure looks good. Uses hooks and resource loader. May want to create a  directory and store css/js files that belong to the same module next to each other in the same directory, but the way it is now is fine too.

(Niklas) I would use folder name resources, and name the files ext.webfonts.css/js and ext.webfonts.fontlist.js.

Done: Context
The principle of contexts is still relatively new in MediaWiki, but I would really recommend making use of them for anything new. See Manual:RequestContext.php, short example: