[videoPlayer] Fluid 4559 - Re-wiring the timeupdate event to control the timing for the updates on time progress and captions (#13)

[Antranig Basman]

Thanks, cindy - looking very good now, and the remaining comments are very minor -

js/VideoPlayer_captionLoader.js line 42
              args: ["@0"]

This style of referring to arguments in IoC configuration is deprecated and will be removed in a coming 
version of the framework. Please use "{arguments}.0" instead

Thanks for the comment on this impl which refers readers to the WebVTT or json format - could we have a link 
which explicitly directs the reader to the standard in question where the definition of this time format 
appears (I hope!) - which is the "json format"?

js/VideoPlayer_intervalEventsConductor.js line 87
        for (var intervalId in intervalList) {

I believe that intervalList is an array (as described in the component comments) and so this form of 
iteration is not one that we encourage. Should our JIRA come back up, you can read discussion on this kind 
of issue at http://issues.fluidproject.org/browse/FLUID-4028

I encourage a replacement of this search for the interval with an idiomatic use of the fluid.find function, 
which guarantees to iterate safely over whatever kind of container the user has supplied.

VideoPlayerIntervalEventsConductorTests.js

Good work on removing all references to the HTML5 driver from this file. I am puzzled about why and how the 
interval Ids become converted to strings, e.g. "0" rather than the numerical indexes that they doubtless are 
in the interval list. I can't easily see where in the implementation of either the component or the test 
cases this occurs.... this shouldn't happen as far as I can see. Can you discover where it happens and 
explain it? (And preferably, make it go away : P)



On 06/02/2012 14:42, Li, Cindy wrote:
> Hi Antranig,
>
> Thanks for taking the time to share the thoughts and explain in such a detail.
>
> Your suggestions have been applied. The pull request for this branch is ready for another round of review,
>
> https://github.com/fluid-project/videoPlayer/pull/13
>
> Cheers!
>
> Cindy
>