User:Catrope/UploadWizard review

Jump to navigation Jump to search

Review as of r84913


  • apiUrl should use $wgScriptExtension
  • Commented-out stuff about exporting blacklist regexes; is that supposed to stay or go?


  • The CanonicalNamespaces thing is ugly, but I understand why you need it (File is the canonical name and we don't do a great job of exporting those to JS ATM AFAIK); it should be fixed properly at some point though
  • Tipsy, autoellipis and jquery.suggestions are in core, don't duplicate them


  • This should honor the MW_INSTALL_PATH environment variable if present, instead of assuming the extensions directory will always be a sibling of the maintenance directory


  • Also makes assumptions about the location of the resources directory relative to $wgExtensionAssetsPath. There's no easy way to fix it, though, and it's just the test suite.


  • This still seems to be used here and there. Is UW not fully RL-ified yet?


  • In mw.UploadWizardDeedChooser() there's some scary HTML building using things like value="' + + '. Please use jQuery functions instead so things are escaped properly.


  • // IE6 is idiotic about radio buttons; you have to create them as HTML or clicks aren't recorded --> does that mean you have to build all the attributes in a scary way? Can't you e.g. create it as <input type="radio" /> and add other attribs later?


  • visibleFilenameDiv seems to be leaked to the global scope
  • function() { _this.upload.remove(); } is redundant, you can just use _this.upload.remove
  • Bindings for mouseenter and mouseleave are commented out, what's up with that?
  • Quick testing seems to indicate that the src attribute of an <img> is already URL-encoded when you fetch it, so you don't need to URL-encode it in setPreview()
  • showTransportProgress() doesn't use its argument


  • Things that are intended to be called as $.foo instead of $('something').foo() should not be registered as $ but as $.foo


  • Use /* @embed */ for icon images.


  • - var query = {}; + q = {}; leaks q to the global scope.
  • To what degree is this library not obsoleted by $.param() ?


  • + var _this = this; // was a triumph
  • + // I'm making a note here
    • After wrestling with bootloaders and reviewing lots of your code, seeing these comments cracked me up. Thank you for making my day :)
  • + 1;
    • that's a valid statement, and does absolutely nothing, but I do wonder what it's doing there :)
  • + var thumbWikiText = "[[" + thumbTitle.replace('_', ' ') + "|thumb|" + gM( 'mwe-upwiz-thanks-caption' ) + "]]";
    • Note that this replace call will only replace the first occurrence. To replace all underscores with spaces, you need .replace( /_/g, ' ' )
  • + var cookieString='skiptutorial=1; expires=' + e.toGMTString() + '; path=/';
    • Did you know we have the jQuery cookie plugin in core?
  • + wikiText = ;
    • More global scope leakage


  • Why are you exporting wgSiteName by hand? If that's not being exported by MediaWiki, something is very wrong.