Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated player.ready to always be async #2188

Merged
merged 1 commit into from
May 23, 2015
Merged

Conversation

heff
Copy link
Member

@heff heff commented May 22, 2015

Addresses #1667

I've changed ready() and triggerReady() to always be async. However now a bunch of track tests are falling.

@gkatsev, can you provide any insight here? I would guess it has to do with the text track display.

player.ready(Fn.bind(this, function() {

Text Track Controls should be displayed when text tracks list is not empty FAILED
  control is displayed
  textTracks contains one item. Expected: 1 Actual: 0

If I move the addRemoteTextTrack part out of the ready listener the tests actually pass, but then other tracks tests fail.

Tracks TextTrackDisplay initializes tracks on player ready FAILED
  TypeError: Cannot read property 'tracks' of undefined

@gkatsev
Copy link
Member

gkatsev commented May 22, 2015

It probably has to do with the workarounds I've done previously to get around the fact that html5 is sync and flash is async.

@gkatsev
Copy link
Member

gkatsev commented May 22, 2015

Also, code-wise, LGTM


for (let i = 0; i < readyQueue.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kill those for-loops with extreme prejudice! :D

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

➰ 🔫 🙀

@gkatsev
Copy link
Member

gkatsev commented May 22, 2015

Also, didn't someone say that text tracks are generally broken in master, currently? Possibly related to that as well.

@gkatsev
Copy link
Member

gkatsev commented May 22, 2015

It's possible that we can remove the player.ready() wrapped for tracks and then just remove the test that checks that tracks are created on ready. I think that was one of my workarounds for this.

@gkatsev
Copy link
Member

gkatsev commented May 22, 2015

Ah, I know what the fix for the tests is. We need to actually trigger ready by ticking the clock forward.

@gkatsev
Copy link
Member

gkatsev commented May 22, 2015

I have a PR against this that fixes the text track errors: heff#9

@heff
Copy link
Member Author

heff commented May 22, 2015

Thanks! Nice use of the PR PR. :)

I tried that with one test but only ticked 1ms, which apparently isn't enough even though ready() waits 1ms. Guess it's safer to be more liberal with the time.

Tests are passing now. Attempting to remove the ready listener now.

@heff
Copy link
Member Author

heff commented May 22, 2015

Removed it and it's just the one test breaking now that tests specifically for that (like you said). Killing that test.

@@ -730,22 +731,30 @@ class Component {
* @return {Component}
*/
triggerReady() {

// // Ready should only be fired once
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dmlap, I had this change in there but then decided to hold on it until we figured out how we wanted to handle tech changes. I forget where we landed, but also realized that the readyQueue does reset itself after ready is triggered, so ready listeners won't fire again on a tech change. That is, unless you listen for the ready event, not pass the function to ready(fn).

But basically the ready event was meant to let you add new ready listeners when the tech changes, it just may not be so clear that's the purpose. Maybe a techloaded event is the answer.

@heff heff force-pushed the asyncready branch 2 times, most recently from da2d639 to d3193f5 Compare May 23, 2015 00:25
closes videojs#1667

Fix text track tests.
Now that ready is async, we need to tick the clock so the ready handler
fires.

Remove unneeded ready test
@heff heff merged commit a575801 into videojs:master May 23, 2015
heff added a commit to heff/video.js that referenced this pull request Jul 22, 2015
For 5.0 we switched ready() to always execute listeners
asynchronously, but for most internal use cases we would
prefer the ready function to execute immediately if the
component is already ready.

Fixes videojs#2326
Related: videojs#2382, videojs#1667, videojs#2188
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants