[#928933] Ensure duration value is consistent #356

Open
wants to merge 2 commits into
from

Conversation

Projects
None yet
2 participants
@ScottDowne
Contributor

ScottDowne commented Nov 1, 2013

No description provided.

@@ -369,6 +355,19 @@
// Get video ID out of youtube url
aSrc = regexYouTube.exec( aSrc )[ 1 ];
+ var xhrURL = "https://gdata.youtube.com/feeds/api/videos/" + aSrc + "?v=2&alt=jsonc&callback=?";
+ // Get duration value.
+ Popcorn.getJSONP( xhrURL, function( resp ) {

This comment has been minimized.

Show comment Hide comment
@mjschranz

mjschranz Nov 5, 2013

Contributor

Maybe I'm crazy but shouldn't we wait to do this until after onPlayerReady callback? I'm worried we run into async problem otherwise somehow.

@mjschranz

mjschranz Nov 5, 2013

Contributor

Maybe I'm crazy but shouldn't we wait to do this until after onPlayerReady callback? I'm worried we run into async problem otherwise somehow.

This comment has been minimized.

Show comment Hide comment
@mjschranz

mjschranz Nov 5, 2013

Contributor

Also, we need to handle errors here too incase something implodes. I' m thinking a console.error or console.warn would work.

@mjschranz

mjschranz Nov 5, 2013

Contributor

Also, we need to handle errors here too incase something implodes. I' m thinking a console.error or console.warn would work.

This comment has been minimized.

Show comment Hide comment
@ScottDowne

ScottDowne Nov 5, 2013

Contributor

Yes, it should probably handle errors, I'll do that.

because firstPlay and durationReady are checked before onFirstPlay is fired, it should not be a problem. I did this because we can be waiting for firstPlay and network at the same time. Waiting for flash to process the video to mute, play, pause and unmute can be done while waiting for network responses. Saves a bit of time.

@ScottDowne

ScottDowne Nov 5, 2013

Contributor

Yes, it should probably handle errors, I'll do that.

because firstPlay and durationReady are checked before onFirstPlay is fired, it should not be a problem. I did this because we can be waiting for firstPlay and network at the same time. Waiting for flash to process the video to mute, play, pause and unmute can be done while waiting for network responses. Saves a bit of time.

+ durationReady = true;
+
+ // First play happened first, we're now ready.
+ if ( !firstPlay ) {

This comment has been minimized.

Show comment Hide comment
@mjschranz

mjschranz Nov 5, 2013

Contributor

Wouldn't we want this check to be if it's true and then set it to false, like the onStateChange function?

@mjschranz

mjschranz Nov 5, 2013

Contributor

Wouldn't we want this check to be if it's true and then set it to false, like the onStateChange function?

This comment has been minimized.

Show comment Hide comment
@ScottDowne

ScottDowne Nov 5, 2013

Contributor

That logic for first play has always been kinda backwards, and I don't know why.

It starts as true, and is set to false once done.

I started to think of it as a "are we waiting for first play still", once first play is done, we can turn the flag off.

What we should really be doing is have a function variable for each state in onStateChange, and then we can change the function assigned to that variable. So in the case of firstPlay, it starts as one function, and one inside that function, sets the function variable to another.

Anyway, I'll reverse the logic to a more intuitive position.

@ScottDowne

ScottDowne Nov 5, 2013

Contributor

That logic for first play has always been kinda backwards, and I don't know why.

It starts as true, and is set to false once done.

I started to think of it as a "are we waiting for first play still", once first play is done, we can turn the flag off.

What we should really be doing is have a function variable for each state in onStateChange, and then we can change the function assigned to that variable. So in the case of firstPlay, it starts as one function, and one inside that function, sets the function variable to another.

Anyway, I'll reverse the logic to a more intuitive position.

@@ -615,7 +616,7 @@
duration: {
get: function() {
- return getDuration();
+ return impl.duration();

This comment has been minimized.

Show comment Hide comment
@mjschranz

mjschranz Nov 5, 2013

Contributor

impl.duration is not a function.

@mjschranz

mjschranz Nov 5, 2013

Contributor

impl.duration is not a function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment