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
_______________________________________________________
fluid-work mailing list - [email protected]
To unsubscribe, change settings or access archives,
see http://lists.idrc.ocad.ca/mailman/listinfo/fluid-work
