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

Already on GitHub? Sign in to your account

playbackRate() now works with null player #363

Open
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

aphid commented Dec 10, 2013

I have some test code @ http://aphid.org/funwithnull where it halves playbackRate and restarts every 9 seconds. This will be awesome for rashomon 👯

@ScottDowne ScottDowne commented on an outdated diff Dec 10, 2013

wrappers/null/popcorn.HTMLNullVideoElement.js
@@ -424,6 +429,18 @@
setMuted( self._util.isAttributeSet( aValue ) );
}
},
+
+ playbackRate: {
+ get: function() {
+ return player.playbackRate;
+ },
+ set: function( aValue ) {
+ if (aValue < 0 ) {
+ throw "playbackRate value must be above 0";
+ }
+ setPlaybackRate( aValue );
@ScottDowne

ScottDowne Dec 10, 2013

Contributor

A function is not needed. player.playbackrate = aValue is fine.

I know we use methods like this elsewhere, and you're probably just following that pattern? But I'm really not attached to it and think we should move away from it.

@ScottDowne ScottDowne commented on the diff Dec 10, 2013

wrappers/null/popcorn.HTMLNullVideoElement.js
@@ -424,6 +429,18 @@
setMuted( self._util.isAttributeSet( aValue ) );
}
},
+
+ playbackRate: {
@ScottDowne

ScottDowne Dec 10, 2013

Contributor

html5 media also has a defaultPlaybackRate that always returns 1.0. Probably wouldn't hurt? Not a big deal though.

Contributor

mjschranz commented Dec 10, 2013

Can we also get some tests here for this addition? You mentioned you have some code using this patch already so if there was an easy way to write a reduced test case for this that would be great.

Contributor

ScottDowne commented Dec 10, 2013

I agree some automated unit tests would be ideal, but, it can be annoying writting tests for the first time, so I'm willing to help with that in any way.

Also, very cool because I think this also works in reverse if you set the playbackRate to -1.5. Just a cool little feature.

aphid added some commits Dec 10, 2013

Contributor

aphid commented Dec 13, 2013

I wrote a test which may not be ideal: it passes if it receives a negative playbackRate and the playhead moves backward. This seemed a lot easier than doing complicated timing tests against forward play.

@ScottDowne ScottDowne commented on an outdated diff Dec 13, 2013

wrappers/null/popcorn.HTMLNullVideoElement.unit.js
@@ -27,6 +27,22 @@ var testData = {
ok( video.duration === 123 && video.currentTime === 12.54, "Correct duration and currentTime" );
});
+
+ asyncTest( "Null Wrapper 02 - Null playbackRate testing ", 1, function() {
+
+ nullVid = Popcorn.HTMLNullVideoElement("#video");
@ScottDowne

ScottDowne Dec 13, 2013

Contributor

Might as well change Popcorn.HTMLNullVideoElement to testData.createMedia, same function signature.

Basically the same thing, but where other wrappers have more complex signatures, this one doesn't really need it, but, it keeps things in one spot in case we need to add something.

Contributor

ScottDowne commented Dec 13, 2013

In general the whitespace should be bracket space variable ( "#video" ) vs bracket variable ("#video")

Contributor

ScottDowne commented Dec 14, 2013

I couldn't find a ticket for this in Bugzilla, but, probably should have one before we land.

I filed it here https://bugzilla.mozilla.org/show_bug.cgi?id=950393

And put it into review on myself.

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