[#968874] Disable keyboard controls on YouTube by default. #384

Merged
merged 2 commits into from Mar 4, 2014

Projects

None yet

3 participants

@ScottDowne
Collaborator

No description provided.

@mjschranz mjschranz and 1 other commented on an outdated diff Feb 24, 2014
wrappers/null/popcorn.HTMLNullVideoElement.js
@@ -368,7 +368,9 @@
return getCurrentTime();
},
set: function( aValue ) {
- changeCurrentTime( aValue );
+ if ( aValue !== getCurrentTime() ) {
@mjschranz
mjschranz Feb 24, 2014

Wouldn't it make more sense to put this logic inside the changeCurrentTime function?

@Pomax
Pomax Feb 25, 2014 collaborator

have to agree, this makes more sense as a first check inside changeCurrentTime. The "set" is an implicit request to attempt a timechange, changeCurrentTime can then decide not to change it if it's the same as what it is right now. Although, that said, this check might not even be necessary; returning early means the seek events never fire. Technically correct but possibly not in the spirit of explicitly setting a currenttime

@Pomax Pomax commented on an outdated diff Feb 25, 2014
wrappers/youtube/popcorn.HTMLYouTubeVideoElement.js
},
set: function( aValue ) {
- changeCurrentTime( aValue );
+ if ( aValue !== impl.currentTime ) {
@Pomax
Pomax Feb 25, 2014 collaborator

same comment here. This bypass is probably not a good idea. What aspect of the bug did this solve?

@ScottDowne ScottDowne merged commit 6d16ad4 into mozilla:master Mar 4, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment