User:Krinkle/Extension review/WikiLove

WikiLove extension review by Krinkle, as of r88383.

Structure

 * Modules are preferably stored in a /modules folder or, like:
 * /extensions/WikiLove/modules/
 * ext.wikiLove/
 * ext.wikiLove.js
 * ext.wikiLove.css
 * ext.wikiLove.defaultTypes.js
 * jquery.elastic/
 * jquery.elastic.js

Code style
The PHP coding looks good and in harmony with our conventions. Not much to say about that. The JavaScript coding style looks mostly okay although some deprecated methods are used (accessing legacy globals for instance). Check out RL/JD, Code_conventions and Code_conventions out to be sure.

WikiLove.js
Right now all these are publically available as object members of, I'm not sure which are supposed to be editable by local wikis and which not, but I suggest putting actual (editable) configuration options in an "options" object to make it easier for users to understand what they can edit. This also enables us to later (see MediaWiki:WikiLove.js) limit the local wiki's hook to only only this (and not the rest of the object, ie. overwrite $.wikiLove.openDialog or something crazy like that) Loading through  isn't synchronous per se, if later code depends on it (ie. overriding configuration variables) it should be loaded with a callback to the rest of the code. This could be done through, however since it's not an external script, it may as well  be registered as part of a module. (ie.  which would be a ResourceLoaderWikiModule loading ). The callback can then be passed to - This module would only declare the $.wikiLove object and not 'do' anything yet. - ext.wikiLove.core.js</tt> would contain something like  and This wiki page may (if local wiki decides to) contain. This file would call, define  and pass   to jQuery's document ready hook. I see several single-image wrappers which I think may be better done through CSS with classes and background images. This also enables ResourceLoader to convert them (when indicated with ) to base64 which saves a few extra HTTP requests. As Roan mentioned, there's a lot of DOM manipulation going on. Take a look at jquery.articleFeedback.js to see how this was done in ArticleFeedback (one main HTML string with  placeholders, then the HTML string is passed to jQuery, then some additional manipulation is done, and finally   to solve the placeholders at once). jQuery's Sizzle engine (like the browsers native CSS engine) goes from right to left. get's all Anchor tags on the page, and the parent is checked of each to match #wlTypes. Aside from that, jQuery has a  detector. Whatever passses that regular expression will be taken care of without initializing the Sizzle engine. A simple ID-selector is one of those quickExpr-things. The following would be many times faster due the above two things (passing quickExpr and only getting the anchor tags within context of the ID-tag): The " " argument passed to callbacks bound through jQuery isn't the native Event object but one normalized by jQuery (see jQuery Event Object). This means preventDefault is always available and will internally fallback to returning false to the event if needed). Returning false from the callback function (unless done for other reasons, such as stopping event bubbling (stopPropagation) or disallowing local wikis to bind events to it) is not needed. When looking back and forth where members are coming from I get the impression currentTypeId (and others a like) may be better suited as local variables within the IIFE rather than (publicly settable and writable) object members. This will also give a slight perforance boost as accessing (deep) object members is slower than accessing (local) variables directly ( vs.  )
 * options
 * MediaWiki:WikiLove.js
 * Alternatively, since it looks like MediaWiki:WikiLove.js is unconditionally loaded, you may want to do this more server side to avoid making an HTTP request half-way $.wikiLove.openDialog:
 * Register  (jquery.elastic.js</tt>)
 * Register  (ext.wikiLove.core.js</tt>, ext.wikiLove.css</tt> and ext.wikiLove.defaultTypes.js</tt>, depending on jquery.elastic</tt>).
 * Register  (ResourceLoaderWikiModule: MediaWiki:WikiLove.js</tt>, depending on ext.wikiLove.startup</tt>)
 * Register  (ext.wikiLove.init.js</tt>, depending on ext.wikiLove.local</tt>).
 * Add  to output modules, the rest will be taken care of by resourceloader and all still in a single load.php request.
 * wgExtensionAssetsPath<br/ >Referring to static files (like images) should be done through  not through  . Some wikis override this path (such as Wikimedia). But also many users that run a wiki off trunk, that have /trunk/extensions</tt> and /trunk/phase3</tt> and alias the former to /trunk/phase3/extensions/my-symbolic-link</tt>), who set   to that.
 * Interface images
 * Building HTML
 * Selectors
 * preventDefault
 * preventDefault
 * Storage & settings (local & public)