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

[Antranig Basman]

Thanks cindyli - this implementation is looking very close. Only some minor comments:

js/VideoPlayer_intervalEventsConductor.js:

i) The finalInit function at line 63 is redundant, the standard model grade automatically creates an applier 
(you will need to add the model grade!)

tests/js/VideoPlayerIntervalEventsConductorTests.js

ii) The unit tests for this component should not use the HTML5 media element (which will probably not run on 
IE8 etc.) - this aspect is already taken care of by the integration tests for this component. This test case 
should use a synthetic, test-driven driver for the conductor as we discussed.

iii) l87, 110: "success" and "error" are probably misleading names for these test cases since they are not 
really testing any failure of the component - rather they are testing the change and non-change of the 
interval.

js/VideoPlayer_captionLoader.js

iv) Thanks for the improved commenting and returned public implementation for "convertToMilli". Where do 
such timestamps come from? Is it from one of the standard subtitle file formats? If so, this should be 
documented, and the conversion function parameterised in the component rather than hardcoded in the impl at 
lines 77 etc.


Component is looking pretty good now!
Cheers,
A


On 01/02/2012 09:00, Cindy Qi Li wrote:
> @amb26 @colinbdclark : This pull request is ready again for the review. Thanks.
>
> ---
> Reply to this email directly or view it on GitHub:
> https://github.com/fluid-project/videoPlayer/pull/13#issuecomment-3761137