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

refactor: player.listenForUserActivity_() #4719

Merged
merged 2 commits into from Nov 16, 2017

Conversation

Projects
None yet
3 participants
@kocoten1992
Copy link
Contributor

kocoten1992 commented Nov 4, 2017

Description

Change setTimeout min to 4 as https://www.w3.org/TR/2011/WD-html5-20110525/timers.html#timers

Requirements Checklist

  • refactor: player.listenForUserActivity_()
@gkatsev
Copy link
Member

gkatsev left a comment

I understand the change to 4 but the meaning of this number is different than a pure timeout number.

The rest of the refactor is great.


// Min time for setTimeout is 4ms regardless any less number
// https://www.w3.org/TR/2011/WD-html5-20110525/timers.html#timers
if (timeout <= 4) {

This comment has been minimized.

@gkatsev

gkatsev Nov 7, 2017

Member

I think we want to keep the 0 check here because while the timer does resolve to 4ms, the value 0 has the special meaning of stopping the inactivityTimeout. A number that isn't zero means that you want a timeout. I think it would be confusing if you set the inactivityTimeout to 2 and then have it not function.

This comment has been minimized.

@kocoten1992

kocoten1992 Nov 7, 2017

Contributor

Hi, does that mean timeout === 0 good enough or it should be at normal timeout < 0, I'm in flavor the first if just 0 if treat it as special

This comment has been minimized.

@gkatsev

gkatsev Nov 7, 2017

Member

unfortunately, we should keep the same check because it could break people. We can correct this in a major update.

@gkatsev

gkatsev approved these changes Nov 7, 2017

Copy link
Member

gkatsev left a comment

Thanks @kocoten1992!

@gkatsev gkatsev removed the needs: updates label Nov 13, 2017

@misteroneill misteroneill added confirmed and removed needs: LGTM labels Nov 13, 2017

@gkatsev gkatsev merged commit c16fedf into videojs:master Nov 16, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment