User "Catrope" posted a comment on MediaWiki.r93351.
Full URL:
https://secure.wikimedia.org/wikipedia/mediawiki/wiki/Special:Code/MediaWiki/93351#c20628
Commit summary:
AjaxCategories rewrite:
Solving syntax problems, performance improvements and applying code conventions:
* Replaced sprite image with separate images and letting ResourceLoader embed
them with @embed (@embed means 0 http requests, less maintenance, none of the
known limitations with sprites, and more readable code (named files rather than
pixel offsets)
* Many functions were floating in the global namespace (like
window.makeCaseInsensitive). A statement ends after a semi-colon(;). All
functions declared after "catUrl" were assigned to the window object. I've
instead turned the semi-colons back into comma's, merged some other var
statements and moved them to the top of the closure. Changed local function
declarations into function expressions for clarity.
* fetchSuggestions is called by $.fn.suggestions like ".call( $textbox,
$textbox.val() )". So the context (this) isn't the raw element but the jQuery
object, no need to re-construct with "$(this)" or "$(that)" which is slow and
shouldn't even work. jQuery methods can be called on it directly. I've also
replaced "$(this).val()" with the value-argument passed to fetchSuggestions
which has this exact value already.
* Adding more function documentation. And changing @since to 1.19 as this was
merged from js2-branch into 1.19-trunk and new features aren't backported to
1.18.
* Optimizing options/default construction to just "options = $.extend( {},
options )". Caching defaultOptions is cool, but doesn't really work if it's in
a context/instance local variable. Moved it up to the module closure var
statements, now it's static across all instances.
* In makeSuggestionBox(): Fixing invalid html fragments passed to jQuery that
fail in IE. Shortcuts (like '<foo>' and '<foo/>') are only allowed for
createElement triggers, not when creating longer fragments with content and/or
attributes which are created through innerHTML, in the latter case the HTML
must be completely valid and is not auto-corrected by IE.
* Using more jQuery chaining where possible.
* In buildRegex(): Using $.map with join( '|' ), (rather than $.each with +=
'|'; and substr).
* Storing the init instance of mw.ajaxCategories in mw.page for reference
(rather than local/anonymous).
* Applied some best practices and "write testable code"
** Moved some of the functions created on the fly and assigned to 'this' into
prototype (reference is cheaper)
** Making sure at least all 'do', 'set' and/or 'prototype' functions have a
return value. Even if it's just a simple boolean true or context/this for
chain-ability.
** Rewrote confirmEdit( .., .., .., ) as a prototyped method named
"doConfirmEdit" which takes a single props-object with named valuas as
argument, instead of list with 8 arguments.
* Removed trailing whitespace and other minor fixes to comply with the code
conventions.
** Removed space between function name and caller: "foo ()" => foo())
** Changing "someArray.indexOf() + 1" into "someArr.indexOf() !== -1". We want
a Boolean here, not a Number.
** Renamed all underscore-variables to non-underscore variants.
== Bug fixes ==
* When adding a category that is not already on the page as-is but of which the
clean() version is already on the page, the script would fail. Fixed it by
moving the checks up in handleCategoryAdd() and making sure that
createCatLink() actually returned something.
* confirmEdit() wasn't working properly and had unused code (such as
submitButton), removed hidden prepending to #catlinks, no need to, it can be
dialog'ed directly from the jQuery object without being somewhere in the
document.
* in doConfirmEdit() in submitFunction() and multiEdit: Clearing the input
field after adding a category, so that when another category is being added it
doesn't start with the previous value which is not allowed to be added again...
Comment:
<pre>
+ category = new mw.Title( redirect[0].to
)._name;
</pre>
Are you supposed to be accessing private members of mw.Title like that?
<pre>
/**
- * Execute or queue an category add
+ * Execute or queue an category add.
+ * @param $link {jQuery}
+ * @param category
+ * @param noAppend
+ * @param exists
+ * @return {mw.ajaxCategories}
</pre>
The behavior of this function is barely documented, specifically what it does
with <code>$link</code>. In fact, a lot of the internal functions are
undocumented, and that's only OK if they're doing trivial things.
<pre>
+ this.showError( 'Unexpected error occurred. $link
undefined.' );
</pre>
Hardcoded English.
<pre>
+ $( this )
+ .unbind( 'click' )
+ .click( that.handleDeleteLink )
</pre>
Could you document what's going on in the click handler here? You're unbinding
and rebinding the click handler from inside the click handler and doing some
other funky stuff, which one has no chance of understanding unless one is
thoroughly familiar with the interface. I think I sort of understand now, after
reading it a second time, but some comments would be nice.
Also, whenever you're unbinding event handlers, you should namespace them so
you don't end up unbinding someone else's handlers by accident.
<pre>
$link.data( 'deleteButton' ).click();
</pre>
This is just one occurrence, this pattern appears all over the code. Calling
click handlers on UI elements to achieve some action is bad form IMO. Instead,
make the handler public (or at least visible to the code that needs it), and
have other code call it directly.
<pre>
+ $( '#catlinks li a' ).each( function() {
</pre>
Isn't this a big faux pas in your own JSPERF guide?
<pre>
+ $anchor = $( '<a>' )
+ .append( cat )
</pre>
<code>cat</code> is a category name, so you need <code>.text( cat )</code> here
to escape things properly.
<pre>
if ( matches.length > 1 ) {
// The category is duplicated.
// Remove all but one match
for ( var i = 1; i < matches.length;
i++ ) {
oldText = oldText.replace(
matches[i], '' );
}
}
newText = oldText.replace( categoryRegex,
newCategoryString );
</pre>
This can produce inconsistent results. Imagine what happens if, for instance,
<code>matches[2] === matches[0]</code>. This isn't too bad, it's just weird.
Performance also isn't ideal, because the string is searched multiple times. I
think it would be more intuitive to do something like <code>var done = false;
newText = oldText.replace( categoryRegex, function() { if ( !done ) { done =
true; return newCategoryString; } else { return ''; } } );</code>. That would
also avoid the weirdness and the performance concern I described.
<pre>
containsCat: function( cat ) {
cat = $.ucFirst( cat );
return this.getCats().filter( function() {
return $.ucFirst( this ) === cat;
} ).length !== 0;
},
</pre>
Using <code>.filter()</code> like that is cute, but in this case inefficient.
If there are a hundred categories and the one you're looking for is first,
it'll still traverse the other 99. Instead, use <code>.each()</code> to
traverse the list. When you find a match, set a function-scope variable to true
to indicate you found a match, then return false from the each callback to stop
the traversal.
<pre>
$link.parent.remove();
</pre>
That will throw a JS error, because <code>$link.parent</code> is a function.
You need <code>$link.parent().remove()</code>.
<pre>
var infos = reply.query.pages;
</pre>
This'll cause a JS error if, for some reason, the query element is not present
in the response (I'm not sure the API will return such a response in practice;
I guess in theory it could happen if there's like a DB error response or
something), so program defensively and make sure things exist before you access
them.
<pre>
$.each( infos, function( pageid, data ) {
var token = data.edittoken;
var timestamp =
data.revisions[0].timestamp;
var oldText = data.revisions[0]['*'];
// Replace all nowiki and comments with
unique keys
var key = mw.user.generateId();
var nowiki = [];
</pre>
var, var, var, var, var. And a few more a bit further down in the same function.
<pre>
summary = summary.join( '<br/>' );
</pre>
In this case, <code>summary</code> comes from
<code>that.stash.summaries</code>, which is filled with text strings that
aren't escaped for HTML, then treated as HTML by this join here. So that's a
potential XSS vector, or at least something that scares me.
<pre>
summaryHolder = $( '<p>' )
.html( '<strong>' + mw.msg( 'ajax-category-question' )
+ '</strong><br/>' + props.actionSummary );
</pre>
And here <code>props.actionSummary</code> is unescaped text that's thrown into
the HTML, which is scary, as is the fact that the ajax-category-question
message is treated as an HTML message for no good reason.
That's all I have.
_______________________________________________
MediaWiki-CodeReview mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-codereview
