User:Krinkle/Extension review/WebFonts

Frontend

HTML escaping

When using localization messages, be sure to always make sure it is properly escaped to prevent potential html injections as well as preventing malformed markup with special characters.

If using jQuery's .html, use .text( mw.msg( ... ) ) instead of .html( mw.msg( ... ) ). jQuery will make sure to set the elements' inner text value instead of the raw html. This is the best option and is also fastest in performance because it avoids escaping all together because .text() goes almost straight into the browser, removing the need for escaping.
If using jQuery's .append, escape manually .append( '<li>' + mw.message( 'example' ).escaped() + '</li>' );
If manually building an html string, escape manually by creating a message object and calling .escaped() (instead of the mw.msg shortcut, which does mw.message(key).plain() ):
'<foo>' + mw.message( 'example' ).escaped() + '</foo>';

On that note, I noticed a lot of .append calls even though the element being appended to is empty. In that case simply use .text( mw.msg( .. ) ).

Status : Done

CSS Injection

Regarding $.webfonts.loadcss( fontfamily ).

@font-face {
	font-family: 'Miriam CLM';	
	src: url('/mw/trunk/phase3/extensions/WebFonts/fonts/Hebr/MiriamCLM-Book.eot');
	src: local('Miriam CLM'),
		url('/mw/trunk/phase3/extensions/WebFonts/fonts/Hebr/MiriamCLM-Book.woff') format('woff'),

			font-weight: normal;
}

It makes assumptions about which formats a fonts has, and yet at the same time it concatenates everything conditionally. If a font does not have a 'ttf' format, the syntax will be broken for font-weight as there would be a missing semi-colon (which is only concatenated if there is a ttf format available).

A way around this bug is by using an array for that part of the css, and afterwards using join() with a comma and adding a semi-colon at the end.

While at it, you may want to use mw.util.addCSS instead to create and inject style tags. It's a little more efficient/faster for performance.

Status : Done

IE crashes if mw.uti.addCSS is used. Generally it works, except for css with @font-face, More: http://stackoverflow.com/questions/7931658/dynamically-adding-font-face-rule-in-ie-8-and-less - User:Santhosh.thottingal

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:

Uncaught ReferenceError: wgContentLanguage is not defined

Use mw.config instead.

IE crashes if mw.uti.addCSS is used. Generally it works, except for css with @font-face, More: http://stackoverflow.com/questions/7931658/dynamically-adding-font-face-rule-in-ie-8-and-less - User:Santhosh.thottingal

Status : Done

HTML fragments

   .append( $('<a href="#" />').append(mw.msg("webfonts-load")) )

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. ( $( '<div>' ); instead of $('<div />'); ).

Status : Done

Dependencies

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

User options / wgWebFontsEnabledByDefault

Logging

Use mw.log instead of conditionally calling console.log. 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.

Status : Done

jQuery stuff

	//we need to reset the fonts of Input and Select explicitly.
	$("input").css('font-family', $.webfonts.oldconfig["font-family"]);
	$("select").css('font-family', $.webfonts.oldconfig["font-family"]);

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

	$(this).css('font-family', fontFamily);
	$(this).addClass('webfonts-lang-attr');

I see this in another part of the code as well. Use jQuery method "chaining". Like: $(this).css('font-family', fontFamily).addClass('webfonts-lang-attr');

	$( '#'+fontID( selectedFont ) ).attr( 'checked', 'checked' );

You probably mean the checked-property instead of the checked-attribute. Use .prop( 'checked', true ); 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 $(...)[0].checked = true;, later they builded a mapping system into .attr() to automatically update the property as well, however since it's not really an attribute they created .prop() in jQuery 1.6, please use it.

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

	if ( $.inArray(fonts[j], config) === -1 ) {
/* ... */
	if ( $.inArray(cookieFont, config) >=0 ){

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

Status : 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.

// CSS whitespace
- padding:5px;
+ padding: 5px;

- div.webfontMenu:hover div.menu, div.webfontMenu div.menuForceShow {
+ div.webfontMenu:hover div.menu,
+ div.webfontMenu div.menuForceShow {

// JavaScript whitespace
- function foo(bar) { if(bar) { return quux(bar, "foo"); } };
+ function foo( bar ) { if ( bar ) { return quux( bar, 'foo' ); } };

Status : Done

Module

Right now there are two modules:

webfonts.fontlist: This initializes the $.webfonts object and populates the $.webfonts.config property
webfonts: (depends on webfonts.fontlist) Re-initializes the $.webfonts object, selectively keeping some values from the another object. Also performs the on-documentready events.

I'd recommend moving $.webfonts into the mw object instead of the jQuery object. Reason being that it doesn't appear be acting like a general purpose jQuery plugin (such as var x = new $.webFonts( $( '#foobar' ) ); or $( '#foobar' ).webFonts();). Instead it's an object literal with helper methods doing something for MediaWiki, therefor it may be better to store this in mw.webFonts.

The way it works now is good enough. For easier testing later though you may want to consider separating definition from execution as a good practice.

ext.webFonts.core module
- ext.webFonts.js: Defines the mw.webFonts object. Much like the webfonts.js right now, except that config would just be config: { fonts: {], languages: {} }
- ext.webFonts.fontlist.js: Extends mw.webFonts.config object with the list of fonts and languages
- ext.webFonts.css

ext.webFonts.init module
- ext.webFonts.init.js: Depends on ext.webFonts.core. Only binds mw.webFonts.setup to document ready.

Also, set 'position' => 'top' for both of these modules so that they start loading and initializing from the head and only execute after document ready. (instead of now when they start downloading after document ready)

Status : Done

Bugs

font-family:none

Action: View English page when logged-out.
- Reaction OK: No fonts are loaded; No menu appears; DOM remains unchanged
Action: View English page with <span lang="he">foo when logged-out
- Reaction OK: Font 'MiriamCLM-Book' is loaded. Menu appears; Above span gets style="font-family: 'Miriam CLM'; "
- Action: Click Reset
  - Reaction Problem: JavaScript sets style="font-family: none;" (which doesn't exist and browsers like Chrome fallback to an inappropriate serif font). It should instead remove the font-family in order to fallback to how it was before ("reset").

Status : Done

Backend

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 $wgWebFontsEnabledByDefault 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 "$wgWebFontsEnabledByDefault". 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).

Status : Done

File structure

General structure looks good. Uses hooks and resource loader. May want to create a /modules/ 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.

Status : 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:

	/**
	 * BeforePageDisplay hook handler.
	 */
	public static function addModules( $out, $skin ) {
-		global $wgUser;

-		if ( $wgUser->getOption( 'webfontsEnable' ) ) {
+		if ( $out->getUser()->getOption( 'webfontsEnable' ) ) {
			$out->addModules( 'webfonts' );
		}

		return true;
	}

Status : Done