User:Catrope/UploadWizard review

From MediaWiki.org
Jump to: navigation, search

Review as of r84913

UploadWizard.config.php[edit]

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

UploadWizardHooks.php[edit]

  • 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[edit]

  • 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[edit]

  • 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[edit]

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


resources/mw.UploadWizardDeed.js[edit]

  • 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[edit]

  • // 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[edit]

  • 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[edit]

  • 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[edit]

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

resources/mw.Uri.js[edit]

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

resources/mw.UploadWizard.js[edit]

  • + 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[edit]

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