-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
First part of the BCD for the Web Audio API #433
Conversation
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.
Overall really good, beside a few specific things, most comments are generic (how we do constructors, interface-level info, how we do alternate names, where we indicate prefixing, ordering of browsers).
api/AnalyserNode.json
Outdated
"AnalyserNode": { | ||
"__compat": { | ||
"mdn_url": "https://developer.mozilla.org/docs/Web/API/AnalyserNode/AnalyserNode", | ||
"support": { |
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.
Can you put the support content in this order:
"webview_android"
"chrome"
"chrome_firefox"
"edge"
"edge_mobile"
"firefox"
"firefox_android"
"ie"
"ie_mobile"
"opera"
"opera_android"
"safari"
"safari_ios"
It makes it much easier to review (similar values are kept together), is closer to alphabetical order (except webview), and is easier to update values (desktop and mobile often change at the same time).
api/AnalyserNode.json
Outdated
{ | ||
"version_added": "14", | ||
"version_removed": "34", | ||
"prefix": "-webkit-" |
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.
The prefix is "webkit" leading to webkitfftSize and not to -webkit-fftSize.
api/AnalyserNode.json
Outdated
}, | ||
{ | ||
"version_added": "14", | ||
"version_removed": "34", |
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.
Is the prefix no more supported ("34") or supported in parallel (no "version_removed" entry)?
api/AnalyserNode.json
Outdated
{ | ||
"version_added": "15", | ||
"version_removed": "21", | ||
"prefix": "-webkit-" |
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.
The prefix is "webkit".
api/AnalyserNode.json
Outdated
}, | ||
{ | ||
"version_added": "15", | ||
"version_removed": "21", |
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.
"21" or no version_removed?
api/AudioListener.json
Outdated
}, | ||
"firefox_android": { | ||
"version_added": false, | ||
"notes": "Still supports the older way of setting orientation — the <code>setOrientation()</code> method." |
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.
Ditto
api/AudioListener.json
Outdated
}, | ||
"firefox": { | ||
"version_added": false, | ||
"notes": "Still supports the older way of setting orientation — the <code>setOrientation()</code> method." |
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.
Ditto
api/AudioListener.json
Outdated
}, | ||
"firefox_android": { | ||
"version_added": false, | ||
"notes": "Still supports the older way of setting orientation — the <code>setOrientation()</code> method." |
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.
Ditto
api/AudioScheduledSourceNode.json
Outdated
{ | ||
"version_added": "25", | ||
"version_removed": "52", | ||
"notes": "Interface implemented as AudioSourceNode." |
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.
This should be on the interface compat status, and using the "alternate_name" methodology
api/AudioScheduledSourceNode.json
Outdated
{ | ||
"version_added": "22", | ||
"version_removed": "43", | ||
"notes": "Interface implemented as AudioSourceNode." |
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.
Ditto (I'm not repeating this)
… interface level compat data
OK, I'm not finished yet, but I have done most of the work. Most of the stuff left to do is around the webkit vendor prefixes. Thinking back on it, I think I might have got this all totally wrong, when I wrote all these compat tables way back when I originally documented the WAA. I reckon the only parts of the WAA that were prefixed when Chrome first implemented it were AudioContext and OfflineAudioContext, These were the only entry point to the API back then — you'd create any other node types with audioCtx.createGain or whatever (these days such nodes have constructors, but you still probably wouldn't use them). So I reckon I can get rid of most of this stuff. I've had a good look on the internet, and I can't find any reference to other prefixed WAA interfaces, or methods and properties. I am however surprised that noone would ever have pointed out such errors in our support data over all these years. what do you think? |
I think you are right, only the initial access point needed a prefix if I recall correctly. |
OK, time to nuke some prefix information then ;-) |
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.
Yes, we are getting closer! This time we are dealing with the meat of this review :-)
api/AudioBufferSourceNode.json
Outdated
"description": "<code>AudioBufferSourceNode()</code> constructor", | ||
"support": { | ||
"webview_android": { | ||
"version_added": "55" |
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.
Missing note: "Before version 59, the default values were not supported."
api/AudioBufferSourceNode.json
Outdated
"version_added": "55" | ||
}, | ||
"chrome": { | ||
"version_added": "55" |
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.
Missing note: "Before version 59, the default values were not supported."
api/AudioBufferSourceNode.json
Outdated
"version_added": "55" | ||
}, | ||
"chrome_android": { | ||
"version_added": "55" |
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.
Missing note: "Before version 59, the default values were not supported."
"version_added": true | ||
}, | ||
"firefox": { | ||
"version_added": "25" |
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.
Is comment [2] at https://developer.mozilla.org/en-US/docs/Web/API/AudioBufferSourceNode/buffer still applicable. (If so add it, if not, forget it, or indicate when it was fixed)
"version_added": "25" | ||
}, | ||
"firefox_android": { | ||
"version_added": "26" |
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.
Ditto.
api/AudioScheduledSourceNode.json
Outdated
{ | ||
"version_added": "14", | ||
"version_removed": "56", | ||
"notes": "Interface implemented as AudioSourceNode." |
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.
Ditto
api/AudioScheduledSourceNode.json
Outdated
{ | ||
"version_added": "25", | ||
"version_removed": "52", | ||
"notes": "Interface implemented as AudioSourceNode." |
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.
Ditto
api/AudioScheduledSourceNode.json
Outdated
{ | ||
"version_added": "25", | ||
"version_removed": "52", | ||
"notes": "Interface implemented as AudioSourceNode." |
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.
Ditto
api/AudioScheduledSourceNode.json
Outdated
{ | ||
"version_added": "15", | ||
"version_removed": "43", | ||
"notes": "Interface implemented as AudioSourceNode." |
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.
Ditto
api/AudioScheduledSourceNode.json
Outdated
{ | ||
"version_added": "15", | ||
"version_removed": "43", | ||
"notes": "Interface implemented as AudioSourceNode." |
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.
Ditto
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.
2 small changes are still to be done (7 times for the second one, but they are extremely tiny.
With these fixed, LGTM, r+
api/AudioContext.json
Outdated
}, | ||
"options": { | ||
"__compat": { | ||
"description": "Support for the <code>options</code> parameter.", |
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.
Drop the "Support for the"; we are describing the compat, so for each subfeature we list if it is supported or not, it is redundant. Just plain "options
parameter" (no dot either, it isn't a note).
api/AudioScheduledSourceNode.json
Outdated
@@ -11,7 +11,7 @@ | |||
{ | |||
"version_added": "14", | |||
"version_removed": "56", | |||
"notes": "Interface implemented as AudioSourceNode." | |||
"alternative_name": "Interface implemented as AudioSourceNode." |
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.
Alternative_name is not a sentence; currently it doesn't even appear as a note in the generated table on mdn, but this name is used to build as structure like: 14-56 as AudioSourceNode.
So "alternative_name": "AudioSourceNode".
api/AudioScheduledSourceNode.json
Outdated
@@ -21,7 +21,7 @@ | |||
{ | |||
"version_added": "14", | |||
"version_removed": "56", | |||
"notes": "Interface implemented as AudioSourceNode." | |||
"alternative_name": "Interface implemented as AudioSourceNode." |
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.
"alternative_name": "AudioSourceNode".
api/AudioScheduledSourceNode.json
Outdated
@@ -31,7 +31,7 @@ | |||
{ | |||
"version_added": "14", | |||
"version_removed": "56", | |||
"notes": "Interface implemented as AudioSourceNode." | |||
"alternative_name": "Interface implemented as AudioSourceNode." |
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.
"alternative_name": "AudioSourceNode".
api/AudioScheduledSourceNode.json
Outdated
@@ -47,7 +47,7 @@ | |||
{ | |||
"version_added": "25", | |||
"version_removed": "52", | |||
"notes": "Interface implemented as AudioSourceNode." | |||
"alternative_name": "Interface implemented as AudioSourceNode." |
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.
"alternative_name": "AudioSourceNode".
api/AudioScheduledSourceNode.json
Outdated
@@ -57,7 +57,7 @@ | |||
{ | |||
"version_added": "25", | |||
"version_removed": "52", | |||
"notes": "Interface implemented as AudioSourceNode." | |||
"alternative_name": "Interface implemented as AudioSourceNode." |
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.
"alternative_name": "AudioSourceNode".
api/AudioScheduledSourceNode.json
Outdated
@@ -73,7 +73,7 @@ | |||
{ | |||
"version_added": "15", | |||
"version_removed": "43", | |||
"notes": "Interface implemented as AudioSourceNode." | |||
"alternative_name": "Interface implemented as AudioSourceNode." |
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.
"alternative_name": "AudioSourceNode".
api/AudioScheduledSourceNode.json
Outdated
@@ -83,7 +83,7 @@ | |||
{ | |||
"version_added": "15", | |||
"version_removed": "43", | |||
"notes": "Interface implemented as AudioSourceNode." | |||
"alternative_name": "Interface implemented as AudioSourceNode." |
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.
"alternative_name": "AudioSourceNode".
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.
\o/
wooohooo \o/ |
These notes have been in BCD since the very beginning: mdn#433 Chrome 55 is the right version for the constructors overall: https://storage.googleapis.com/chromium-find-releases-static/579.html#57909df69be0cf0b2ac42e5c9c018d4a00ee9766 The Chrome 59 change is this one: https://storage.googleapis.com/chromium-find-releases-static/ef0.html#ef006156fd62853a893617ca104f4ac2045e19c1 Adding default values to dictionary members is by itself not necessarily a web observable changes at all. This was confirmed with one case comparing Chrome 58 and 59: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=10708 One part of this was observable, with the OscillatorNode constructor: https://bugs.chromium.org/p/chromium/issues/detail?id=710471 This was a spec bug, and fixed in the spec around the same time: WebAudio/web-audio-api#1173 Overall, this change does not seem noteworthy enough to record in BCD.
These notes have been in BCD since the very beginning: #433 Chrome 55 is the right version for the constructors overall: https://storage.googleapis.com/chromium-find-releases-static/579.html#57909df69be0cf0b2ac42e5c9c018d4a00ee9766 The Chrome 59 change is this one: https://storage.googleapis.com/chromium-find-releases-static/ef0.html#ef006156fd62853a893617ca104f4ac2045e19c1 Adding default values to dictionary members is by itself not necessarily a web observable changes at all. This was confirmed with one case comparing Chrome 58 and 59: http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=10708 One part of this was observable, with the OscillatorNode constructor: https://bugs.chromium.org/p/chromium/issues/detail?id=710471 This was a spec bug, and fixed in the spec around the same time: WebAudio/web-audio-api#1173 Overall, this change does not seem noteworthy enough to record in BCD.
No description provided.