setSrc clears currentSource_ after loadstart #3285
Conversation
|
Fixes #1951. This still needs disposing the source handler, right? |
|
@gkatsev It has |
|
Moved |
| // Remove the addEventListener alias before calling Events.on | ||
| // so we don't get into an infinite type loop | ||
| let ael = this.addEventListener; | ||
| this.addEventListener = Function.prototype; |
There was a problem hiding this comment.
We have encountered issues with using Function.prototype as a no-op due to the way video.js tracks functions by a GUID property. I don't think it would be an issue here, but I think it's safer to never use it. Unfortunately. 😞
| @@ -11,7 +11,7 @@ EventTarget.prototype.on = function(type, fn) { | |||
| // Remove the addEventListener alias before calling Events.on | |||
| // so we don't get into an infinite type loop | |||
| let ael = this.addEventListener; | |||
| this.addEventListener = Function.prototype; | |||
| this.addEventListener = () => {}; | |||
There was a problem hiding this comment.
Point for discussion: should we be careful about using fat-arrows all over the place? I know calling addEventListener() with a non-this receiver is a weird thing to do but we could use a plain function here and avoid possibly subverting people's expectations.
There was a problem hiding this comment.
We're gonna change this to function() {}
There was a problem hiding this comment.
I don't think it matters here at all since we just want an empty function while Events.on runs so we don't get into an infinite loop, so, there's no expectations being subverted here.
As for elsewhere, it really depends on what you want the context of your function to be since in arrow functions this and arguments are never bound.
There was a problem hiding this comment.
That's the crux of the discussion I think we should have. Using fat-arrows removes the ability of the caller to rebind this. I've felt it necessary to rebind this more than once in my life. Are we intentionally removing that ability or just doing it to save some keystrokes?
gkatsev
added
minor
|
That looks pretty good to me. @dmlap can you re-review? |
|
Oh neat, github picked up that it's the same code (I pushed this branch to the repo so that it can run in browserstack). |
| _Tech.prototype.disposeSourceHandler = function(){ | ||
| // On the first loadstart after setSource | ||
| _Tech.prototype.firstLoadStartListener_ = function() { | ||
| this.one(this.el_, 'loadstart', _Tech.prototype.successiveLoadStartListener_); |
There was a problem hiding this comment.
Why use one() if you want the successiveLoadStartListener_ to run on every subsequent loadstart?
There was a problem hiding this comment.
It's because disposeSourceHandler removes it. We work around this by re-adding it in successiveLoadStartListener_ after disposeSourceHandler has been called.
|
I'm not in love with the solution but I can live with it. LGTM |
Changelog is at 5.9.0 * https://github.com/videojs/video.js/releases/tag/v5.9.0 5.9.1 * https://github.com/videojs/video.js/releases/tag/v5.9.1 5.9.2 * https://github.com/videojs/video.js/releases/tag/v5.9.2 Microsoft Edge change * updated IS_CHROME to not be true on MS Edge 5.10.1 *https://github.com/videojs/video.js/releases/tag/v5.10.1 Important: Changing resolution now works as of this change videojs/video.js#3285 Which is in 5.10.1. Ive tested and changing resolution now works. Bug: T131544 Change-Id: I191d92f24609b22424c402d0ef7f4b44cf1e6b20
Description
currentSrc will now report correct source after the video source was changed by a third party.
This PR replaces #2957
Specific Changes proposed
Requirements Checklist