User:Catrope/Extension review/TimedMediaHandler

Revisions

 * This review is as of r84443; line numbers refer to that rev unless specified otherwise.
 * r84444: tweaked grammar in doc comment
 * r84452: add some /* @embed */
 * r84453: some indentation fixes (spaces->tabs mostly)

WMF deployment questions

 * What kind of thumb URLs does this produce? The WMF 404 handler can already deal with OggHandler's thumb URLs, which look like  or  , where   may also be   or  . The thumbnails on prototype seem to stick to this format, but then I'm not sure what will happen in Firefox 4 with WebM support. Will this extension need any new thumb URL formats to be handled when deployed to WMF?
 * Where do we hook in to update this? .. The "thumbs" path from Timed Media Handler is identical to OggHandler, except we also support webm "source" files. so  should work. NOTE currently neither OggHandler or TimedMediaHandler supports scaled image thumbnails both return the image as the size of the source video regardless of transform request. I will look into adding support for small thumbnails into  TimedMediaHandler
 * Added scaled thumbnail support in 84561 Mdale


 * Does this extension obsolete/replace Extension:OggHandler?
 * Yes this would replace OggHandler. Parts of OggHandler are included in TimedMediaHandler.

Stuff I need help with reviewing

 * Basically all of the JS; there's a shit ton of it. Maybe get Neil to help, or just do a skim for security rather than a 'real' review
 * The media backend stuff is kind of over my head, would like Tim or Bryan to look over it

TimedMediaHandler.php

 * Configuration should be near the top of this file. Right now it starts all the way down on line 72
 * done in r84562


 * Lines 18-22: the magic behavior of adding  to   (in this file) and   (in TMHHooks.php)
 * can be accomplished with
 * done r84563


 * doesn't work for any extensions added to that var in LocalSettings.php
 * we don't want extensions adding to this var for now.. maybe in the future we support other formats but for now just a variable specific to tmh. --21:40, 22 March 2011 (UTC)Mdale


 * if this is intended to not work, that should be documented and it should be made clear this var is not a config var
 * if this is intended to work, the code for merging this var into  should be in an extension function and the var should be documented as a config var and put in the config section
 * Lines 33-36: messing with  should only be done if it's really, really, really necessary. Since it seems to do this for a PEAR module I can imagine that it might indeed be needed, but if there's a way this could be done using regular autoloading or whatever that would be greatly preferable.
 * This is copied from Tim's OggHandler, Will wait for comment from him before removal


 * Lines 101-104: inconsistent casing between  and
 * fixed case in r84564

TimedMediaHandlerHooks.php

 * Lines 34-49: you can move the  part into the base path if you like, but either way is fine
 * Lines 68-103: dynamic namespace number determination scares me, mostly because it makes adding a custom namespace have a really weird effect (all TimedText pages will appear to have been moved into the new custom namespace ). Instead, choose a constant, high number like 700 and register it. This way you also don't need to dynamically look for the TimedText namespace in TextHandler.php, and the name "TimedText" will be localizable

ApiQueryVideoInfo.php
PHP 5.3, hurry! :) Patches for making ApiQueryImageInfo more extensible are welcome. Currently all we have AFAIK is the ApiAfterExecute hook; you'd have to hook into that, parse the result object, recreate the File objects from that and add your properties. It's not as much trouble as it may sound like, but it's not the nicest thing ever and doesn't fix the  issue. While we wait for PHP 5.3, a nice hack could be to wrap those static functions in ApiQueryImageInfo in non-static member functions and extend those.

TimedMediaHandler_body.php

 * Lines 13-15: according to the documentation in the base class,  should at least return something (array or false)
 * Line 21:  was deprecated like a year ago. It can safely be removed as you're obviously not shooting for compat with 1.15 (given that you're using ResourceLoader) :)
 * Lines 86-92: use getter instead of private member access
 * Line 130: document what  does and what NPT stands for
 * Lines 159, 169, 170 : superfluous pair of parentheses around the isset call
 * Line 174: isVideo is a boolean for sure (cause it's built using the  operator) so you can just use   or reduce the amount of indirection and just use   instead of indirectly using

TimedMediaTransformOutput.php

 * Line 60: another wfLoadExtensionMessages
 * Line 115: all of these globals are unused
 * Lines 157, 158: superfluous parentheses around $sizeOverride

TimedMediaIframeOutput.php

 * Lines 16-33: I think it's better to do this with ?action=embedplayer, because MW provides a clean way to override actions instead of your hackery with ArticleFromTitle (of all hooks). It would also save you from having to prevent MediaWiki from doing the default page view action using various hacks like you are now
 * Lines 27-29: if you want to prevent other output,  should be good enough. It's also cleaner than duplicating the end of index.php and blowing up profiling (lots of unclosed profiling sections!) in the process
 * Line 46: why do you need a clean OutputPage object? I think you might no longer need this if you use a decent action override as mentioned above, but you should at least document why you're doing this
 * Lines 83, 90: mixing $j and $

WebVideoTranscode.php

 * Line 130: A common strategy in MW seems to be to use tempnam with wfTempDir (Import.php does this), or have a configurable temp path in a $wg var (includes/media/Bitmap.php does this with $wgImageMagickTempDir

TimedText.loader.js

 * Line 17: don't use $j, use $
 * Line 33: abrupt end of comment mid-sentence
 * Line 41: you're using a possibly asynchronous event to check for something synchronously? This scares me. What's going on here?
 * Does any of this stuff actually need to be a loader script? AFAICT it's not determining the dependencies of another module or anything, it's just setting stuff up. Is there any reason this has to happen early?

RemoteMwTimedText.js

 * Line 26: missing semicolon. This'll work because of semicolon insertion, but that's a scary "feature" of JS that should not be relied upon
 * Line 60: doesn't this mean that every player has the same ID? Isn't that invalid?
 * Line 113:  is misspelled
 * Line 150: so the width part of the default size is respected but the height part isn't?
 * Line 184:  is never gonna be less than zero, so this loop aborts immediately

TimedTextEdit.js

 * Lines 5-30: why are there hardcoded English messages here? ResourceLoader takes care of i18n and message exports for you
 * this file is huge so I didn't review the rest of it just yet

EmbedPlayer.loader.js

 * Lines 97, 99: skinname is leaked to the global scope
 * Same as with the other loader script: I don't see why any of this needs to be in a loader script