Reading/Multimedia/TimedMediaHandler review notes 2011

mw.EmbedPlayer.js
reading from line 459 of mw.EmbedPlayer.js

triggers updateFeatureSupportEvent, with property this.supports. Seems to be unused in this version of the code
 * setupSourcePlayer -- pick a defaultPlayer from the mediaPlayers for the mimeType
 * inheritEmbedPlayer <- why inheritance, why not composition or dispatch? And furthermore why does it blank methods, does that mean this effectively changes type, multiple times during a session? Seems like it would be very hard to debug later...
 * This is rarely used  as more browsers support html5. ( You rarely switch player types ) Inheritance made it easy to override core methods with per embedPlayer type modes of operation. There is enough shared functionality and the player interface implementations are relatively small that it may be worth while to break off "browser playback" implementations from the core "player" and do something similar to controlBuilder.
 * updateFeatureSupport
 * Used in KalturaSupport here .. to update player features per kaltura uiConf configuration.


 * getDuration // lines 568, pointless?! It doesn't assign to anything, do we expect getDuration to have side effects? if there's a reason, please comment
 * Its a getter for the duration, in later version of code it looks for the selectedSource duration (if not deifned in the parent EmbedObject ) copied in r103525


 * naming and/or code structure -- inheritEmbedPlayer does more than inherit it also loads the player, with callback to show the player.
 * Renamed to "updatePlaybackInterface" in r103528 to better capture what the method is doing. Updating the interface in which in browser playback api calls are made.


 * need for CPS? Why not break into multiple functions?
 * I assume you mean Continuation-Passing Style, This is not as bad as some other examples. I think we should focus on the egregious ones that actually make things hard to follow .. with small CPS's I don't think it hurts readability as it makes it easier to track the closure contexts of parent variables without having to explicitly pass them sets of properties across local functions.


 * getTimeRange: coding standards, please use braces
 * r103529


 * wtf with the "do" methods. doEmbedHTML, doSeek, why not embedHTML, seek.
 * renamed doEmbedHTML to embedPlayerHTML and doSeek to seek in r103531


 * naming is PlayerControlBuilder a control or a builder? How can it have a height.
 * PlayerControlBuilder builds controls .. the controls it builds have a height, should be accessed via getHeight
 * line 807, line 850 setTimeout(... 1) ? not 0? line 878
 * fixed to 0 r103532


 * spelling: doWarningBindinng (everywhere)
 * renamed to addWarningBinding r103533


 * doneLoading or trigger( 'playerReady' ) - pick one
 * lots of stuff binds to playerReady so name the property the same ) r103536 Should be noted that propagating w3c player states is on the todo list


 * updateVideoTimeReq -- camel case params. also document format...
 * updateVideoTime -- camel case params
 * fixed camel case, added format documentation in comment r103539


 * line 981 - reset seek_offset (comment is wrong? should be serverSeekTime ?? )
 * Fixed comment and switched check to supportsURLTimeEncoding call in r103539

2011-10-16 Sunday noticed this while debugging other things:
When enabled, TMH seems to cause problems with the documentation produced by an invocation of api.php without parameters. Fatal error: Access level to ApiQueryVideoInfo::getExamples must be public (as in class ApiQueryImageInfo) in /Users/neilk/Sites/trunk/extensions/TimedMediaHandler/ApiQueryVideoInfo.php on line 238
 * fixed in r103540

--

2011-10-17

 * line 1046-1049 braces please, camelcase mixing,
 * fixed in r103544
 * 1084-1086 -- setting css of removed img?!
 * the image fades in then replaces the other image made that more clear by using a single chained set of calls: r103545
 * 1090 thumbUrl - not guaranteed to show all thumbs, just looks at last_thumb_url, is this s et by another process?
 * that is correct .. we just hot swap at the end to lastThumbUrl this was for a metavid.org interface where temporal clips updated the thumbnail.. if you quickly moved to a new image we did not want to queue up lots of animations for each thumbs, for a responsive interface we just jump to the latest ( even if its not pretty for that image swamp )


 * 1104-06 -- updatePosterSrc, same name as complex method at 1044?
 * in kaltura trunk version we just have the simple version ( deprecated metavid interface described above ) .. I have swapped that in r103549


 * 1223: videoAttribues
 * 1281: if applyMediaElementBindings is "abstract" as well, it should somehow indicate an error or at least log one
 * Just a warning because not required that player interfaces implement it: 103550


 * 1455: what is _propagateEvents, really? Why would you have a play button that does nothing? debugging?
 * _propagateEvents is used to disable events. Say you start an ad playback and now you don't want the player to send the 'onended' to the player interface rather you want the embedPlayer to look like one continues stream for purposes of interfacing with the player from JavaScript. Other times its used is for managing triggers during times you don't want triggers to populate. Say for example you switch a source, and as part of that source switch the native html5 implementation dispatches a "pause" event, but you don't want that pause event to update the player interface and controls ( like it normally does ) .. so we have this way to turn off event propagation.


 * what is "autoTrigger"ing events? searching web reveals nothing..
 * its a bad name should be do "eventsAutoBubble" ... but the issue was when you call a method like embedPlayer.play .. it would trigger "play" this has been fixed in later versions of jQuery, and I have switched tirgers to not include local method names ( in kaltura trunk ) Copied over in r103551


 * what do you mean by "underling"s ?
 * very private method or variable :P .. I should be more consistent.


 * 1550: another "abstract" method -- should throw error? but at least it does log.
 * We don't want to throw real errors for things like this.. it could just be the player interface does not have an api for volume control.


 * 1577: we understand that stop is like pause, but.... parent_pause?
 * this is a way to check if the player interface implements pause and only call the embedPlayer method ( not an actual pause request on the player interface )... This is not really needed now days. Worked around a bug where some players would not seek if you called pause on them, and when were were "stopping" we want to enter a pause state but not interfere with the player seek to the start. ( but not needed now ) r103554


 * 1677...1711 more "abstract methods" that don't throw errors, instead are accessors to simple object properties like isPaused --> this.paused. So what should we assume -- that these methods *should* throw errors, or, that this refers to an older design and now all subclasses (underlings??) use the same notion of this.paused
 * These methods should not throw errors. They sometimes are not really needed. isPaused does not read actual player interface properties just the attribute. I agree in this case we can just access the "paused" property. r103555

would be nice if we documented notions like "currentTime"... we assume it means the time of the video playing, but is it really? 1773 -- isDuration == 0 divide by zero? so, is there a monitor loop constantly going, and in checkForCurrentTimeSeek, we try to determine if user action has finished and we can seek to new position. Related to code questions above: Possible interface bug -- seems strange to allow movie to continue playing while user has mousedown on playhead. YouTube pauses instead. Is annoying in use cases like "I am near the end, now want to scrub back to beginning" or "I want to see that thing again that I just saw" If the above interface idea makes more sense, why not turn off monitor loop on mousedown over the playhead, and then, on mouseup, seek to new position and restart monitor loop. monitor function -- break this up? updateBufferStatus line 1926 -- why would this ever be greater than 1 ( > 100%)? If this is due to floating point fails, why not track bytes received / expected instead and divide against expected file length... (of course, it's possible the server lied about file length) bufferedPercent -- only dealt with in subclasses? should be two integers in the superclass? (bufferedBytes/expectedBytes)? Or maybe some players only give you % and not such values? line 1986 - what if no currentTime and/or no serverSeekTime ? what if serverSeekTime is 0 or currentTime is 0, does this do the right thing? What if I'm asking explicitly to seek to 0? question, does this chain different kinds of media together? That would explain a lot. like, why do we keep getting autoSelectSource over and over again. Don't we just want it once? in supportsURLTimeEncoding, isn't this done better with a default "no", and mediaSource subclasses provide their own assessment based on config and their capabilities. ___ Stopped at end of mw.EmbedPlayer.js, yay ____ Oct 28, 2011 mwEmbedPlayerGeneric.js - no comments mwEmbedPlayerJava.js - parseInt should have radix (2nd argument of 10) - clean up commented out code - line 103 consider  !( a && b ) - line 192 - getPlayerElement -- remove commented out stuff. Also, the way this is used in the rest of the code it is not a getter because the returned element is ignored. It is like a "cachePlayerElement". Rename, or fix rest of code to obtain element from method? - line 192 - why doesn't it check for existence of getPlayerElement and return that? Does the player element change, so that we have to do jquery lookup each time? - line 234 - does this need to be wrapped in a try/catch, like the play right above? mwEmbedPlayerKPlayer.js - seems that WIkimedia will not use this, since it is SWF based, so, although there are a few WTFs here and there we are not reviewing. Linted it. Seems like it will not actually blow up the world. :) mw.EmbedPlayerNative.js - 102-106 repeated $( '#' + this.pid ) - line 104-108 postEmbedJS twice, can be made simpler with if/else logic? - 126 playerAttribtues -- no abbreviations, please expand tues to Tuesday. - 131 braces for ifs please - 151 useless parens - 183 - arbitrary wait... 100ms? can we have a comment here as to why? - 296 - 50ms seems like it's overdoing it, why not 100ms or 200ms. Also why 40 seconds for delay here, 10 seconds elsewhere? arbitrary. - 317 braces for if - 314 getCurrentTime -- after puzzling it out we think we understand it but this needs better comments. Lots of interwoven lines of execution. - -why is seeked interwoven with setting time, etc. We believe it works like this -- a "seek" is invoked, with a callback to do later. --- this calls setCurrentTime, passing that callback. --- setCurrentTime spins every 50ms until the element readyState > 1 for a maximum amount of time --- then, if currentTime is time, do callback. or, wait for the 'seeked' event to trigger (which will occur because one of our callers already set something in motion), and then do the callback switchPlaySrc (line 387) -- too many embedded timeouts & closures, please rewrite, we can't understand this easily. line 540 @parma delicious JS documentation? 563 braces for ifs line 608 -- this.seeking = false twice -- we understand why but please set a comment otherwise rest of file ok end 5:40 Oct 28 ---