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

Remove method-chaining capabilities for player.currentTime #3704

Closed
dmlap opened this issue Oct 23, 2016 · 6 comments
Closed

Remove method-chaining capabilities for player.currentTime #3704

dmlap opened this issue Oct 23, 2016 · 6 comments

Comments

@dmlap
Copy link
Member

dmlap commented Oct 23, 2016

Method chaining can save some repetitious typing when using the player APIs:

player.currentTime(7).play();

Unfortunately, switching return types between different function invocations confuses Javascript optimizers and often means the method remains de-optimized. For methods that may be called very frequently during player interactions like currentTime, de-optimized functions that take 10s of milliseconds can lead to frame drops.

@dmlap dmlap changed the title Remove method-chaining capabilities for player level getter/setters (e.g. player.currentTime) Remove method-chaining capabilities for player.currentTime Oct 23, 2016
@dmlap
Copy link
Member Author

dmlap commented Oct 23, 2016

Here's a sample timeline profile from Chrome. Note in the bottom pane thatcurrentTime is de-optimized and took over 10ms of self-time:

screen shot 2016-10-23 at 3 00 06 pm

@misteroneill
Copy link
Member

I think we all agree that we should either do nothing or remove all method-chaining capabilities from video.js.

Presuming we remove method-chaining capabilities, one thing we need to discuss is what to do about returning a Promise from play() (which Chrome does) instead of this. There are a few options:

  1. Return whatever the browser (and the tech) return.
  2. Return a promise if the browser returns one. If the browser does not return a promise, but the browser supports promises, return our own pre-resolved promise. Otherwise, return nothing/undefined.
  3. Always return a promise. This would require users to use a promise-providing library like es6-shim or Bluebird for browsers that don't support promises.

@brandonocasey
Copy link
Contributor

I really like solution 2, but maybe we could make it check if Promises are available instead of depending on a browser check. That way if the promise-providing library provides a Promise or a native Promise exists we can use it, but it won't force anyone to use a promise library.

@misteroneill
Copy link
Member

Yep, agreed.

@dmlap
Copy link
Member Author

dmlap commented Dec 4, 2016

Solution 2 sounds right to me, too.

@misteroneill
Copy link
Member

Sounds pretty uncontroversial, then.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants