User:Catrope/UploadWizard review

Review as of r84913

UploadWizard.config.php

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

UploadWizardHooks.php

• 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

test/jasmine/makeLanguageSpec.php

• 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

test/jasmine/SpecRunner.html

• 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.

UploadWizardDependencyLoader.php

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

resources/mw.UploadWizardDeed.js

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

resources/mw.UploadWizardLicenseInput.js

• // 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?

resources/mw.UploadWizardUploadInterface.js

• 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

resources/jquery/jquery.removeCtrl.js

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

resources/uploadWizard.css

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

resources/mw.Uri.js

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

resources/mw.UploadWizard.js

• + 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

SpecialUploadWizard.php

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