Cleanup legacy browser support in libopenal.js. NFC - #27435
Conversation
1bb7d7d to
6944cd8
Compare
| QUEUE_INTERVAL: 25, | ||
| QUEUE_LOOKAHEAD: 100.0 / 1000.0, | ||
|
|
||
| #if MIN_SAFARI_VERSION < 140500 |
There was a problem hiding this comment.
I think its 14.1 for Safari and maybe 14.5 for mobile safari? This is why we use 140500 elsewhere (e.g. libwebaudio.js).
AI seems to agree: Safari 14.1+ (Desktop) and iOS Safari 14.5+: Switched to standard window.AudioContext
There was a problem hiding this comment.
I verified 14.1/14.5 is on MDN.
|
|
||
| var AudioContext = window.AudioContext || window.webkitAudioContext; | ||
| #if MIN_SAFARI_VERSION < 140500 | ||
| var AudioContext = window.AudioContext ?? window.webkitAudioContext; |
There was a problem hiding this comment.
Why does this read window, and above we read globalThis?
There was a problem hiding this comment.
Using globalThis in hasAudioContext avoid crash on startup on node (or other places where window does not exist). If could use window there too and make it include a function instead of a property if you prefer?
There was a problem hiding this comment.
Wait, why would it be good to use window there too? I would expect to use globalThis in all places, as the most supported? (node, workers, etc.)
Also, can we merge these two places together? Instead of a hasAudioContext property it could be AudioContext, and then we can just use it from there?
There was a problem hiding this comment.
The reason I think its OK to use the window. for the usage path is that we expect that code to crash if its missing.
It doesn't really matter if its undefined window object or undefined window.AudioContext contructor, its still the same crash.
The reason I don't really want to merge these is that I want the usage path in the default case to look like new AudioContext().. not new getAudioContext().. it seems cleaner. Maybe I can keep that though .. let me try it.
|
|
||
| var ac; | ||
| try { | ||
| // Only try to pass options if there are any, for compat with browsers that don't support this |
There was a problem hiding this comment.
Which browser versions was this an issue for?
There was a problem hiding this comment.
Its really hard to know for sure but some ancient version of webkitAudioContext didn't not support options passed to it.
However, since we already don't have this change in libwebaudio.js I figure it much be at least as old as 14.5
This brings libopenal.js inline with libwebaudio.js in a couple of ways: - Only support webkitAudioContext on older safari versions. - Drop support for `new AudioContext` that does not accept options. This change should make is easy for us to remove this support once we drop support for Safari < 14.5.
6944cd8 to
f72c9cc
Compare
2 participants
This brings libopenal.js inline with libwebaudio.js in a couple of ways:
new AudioContextthat does not accept options.This change should make is easy for us to remove this support once we drop support for Safari < 14.5.