-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
handle error getter on flash tech better #2515
Conversation
@@ -297,7 +307,7 @@ class Flash extends Tech { | |||
// Create setters and getters for attributes | |||
const _api = Flash.prototype; | |||
const _readWrite = 'rtmpConnection,rtmpStream,preload,defaultPlaybackRate,playbackRate,autoplay,loop,mediaGroup,controller,controls,volume,muted,defaultMuted'.split(','); | |||
const _readOnly = 'error,networkState,readyState,initialTime,duration,startOffsetTime,paused,ended,videoTracks,audioTracks,videoWidth,videoHeight'.split(','); | |||
const _readOnly = 'networkState,readyState,initialTime,duration,startOffsetTime,paused,ended,videoTracks,audioTracks,videoWidth,videoHeight'.split(','); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove error
.
Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED Commit: ff85e12 (Please note that this is a fully automated comment.) |
|
||
// errors we haven't categorized into the media errors | ||
} else { | ||
tech.trigger('error', msg); | ||
tech.trigger('error', errorObj); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tech.trigger('error', errorObj);
is now the same in both places, so it could be moved outside the conditional.
A few notes but otherwise lgtm. If we can figure out why the player and tech were continuing to operate after the tests, with little more effort, it might save us some pains down the road. My best guess is something isn't being disposed properly in the tests. |
Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED Commit: d663dbf (Please note that this is a fully automated comment.) |
Does #2517 replace this one? |
Assuming we like #2517, yes. |
#2517 got merged. Closing. |
The swf doesn't have an error getter and seems like all the info we have about the error is available in the
onError
method. So, instead of trying to callvjs_getProperty
to the swf (which will never work or return a valid value), we'll store the error that we got there and return it in the error getter.One thing I'm not certain off is when to clear out our reference to the error. Seems like when the error getter gets called is a good place to do so because it gets called from the player and player stores its own reference to the error which it clears out as necessary.