Closed autokagami closed 1 year ago
@padenot It looks like returning a dictionary doesn't really work. Do you have any suggestions?
cc: @tidoust I thought you might have some good ideas too. Please feel free to propose them!
One possible solution is to differentiate the options
and the info
.
dictionary AudioSinkOptions {
required AudioSinkType type;
}
interface AudioSinkInfo {
readonly attribute AudioSinkType type;
}
partial interface AudioContext {
[SecureContext] readonly attribute (DOMString or AudioSinkInfo) sinkId;
}
Yeah that would work.
An alternative, which is simpler, is:
dictionary AudioContextOptions {
...
DOMString? sinkId = "";
...
}
partial interface AudioContext {
[SecureContext] readlonly attribute DOMString? sinkId;
}
So the usage for a silent sink (muted AudioContext) would be:
new AudioContext({sinkId: null};
context.setSinkId(null);
console.log(context.sinkId); // null
We lose flexibility and extensibility by not using the options/info
pattern, but we get simplicity instead.
During the TPAC, I vaguely remember @padenot and I had a discussion on why the simpler approach doesn't work out well. I suggest that we give thoughts on this for a couple of more days and make a decision.
As we all know, the confusion around null
can be a problem.
context.setSinkId(null);
console.log(context.sinkId === undefined); // false
console.log(context.sinkId == undefined); // true
if (context.sinkId) {
// Does this mean |sinkId| is not defined yet? Or does it mean it's silent?
}
It's different from being "undefined" because we defined the sink identifier, but in JS comparison it falls back to falsy and it is confusing. Also based on my experience, it's usually better to specify in details than taking a short path which is seemingly "intuitive".
cc/ @jackschaedler - curious about your thoughts on this.
An alternative, which is simpler, is:
dictionary AudioContextOptions { ... DOMString? sinkId = ""; ... } partial interface AudioContext { [SecureContext] readlonly attribute DOMString? sinkId; }
So the usage for a silent sink (muted AudioContext) would be:
new AudioContext({sinkId: null}; context.setSinkId(null); console.log(context.sinkId); // null
We lose flexibility and extensibility by not using the
options/info
pattern, but we get simplicity instead.An alternative, which is simpler, is:
dictionary AudioContextOptions { ... DOMString? sinkId = ""; ... } partial interface AudioContext { [SecureContext] readlonly attribute DOMString? sinkId; }
So the usage for a silent sink (muted AudioContext) would be:
new AudioContext({sinkId: null}; context.setSinkId(null); console.log(context.sinkId); // null
We lose flexibility and extensibility by not using the
options/info
pattern, but we get simplicity instead.
I think what Hongchan mentioned about why not using context.setSinkId(null) is correct. It does become bug-prone if return sinkId as null. I vote +1 for using (DOMString or AudioSinkInfo) as sinkId return type.
I think you might want to simply change the type of the attribute to attribute (string or object) sinkId
, since in this case the attribute just passes the internal slot.
BTW, Oh, actually scrap it. The IDL layer should automatically do that. It should still be somehow frozen to prevent modification if you go setSinkId
doesn't seem to clone the object. What happens if:object
way.
Thanks for the input, @saschanaz!
I still see the benefit of having a defined class like AudioSinkInfo
is slightly better. Other than a simpler interface, what would be the benefit of the object
approach?
Not too much from IDL side except it simply has less code. You also need to make sure you cache and reuse the instance so that this keeps being true:
context.sinkId === context.sinkId // should be true!
(Dictionary type is disallowed as the attribute would always create a new instance and thus make it false.)
Good point. With AudioSinkInfo
option, do we need to enforce [SameObject]
for the sinkId
property?
Yup, that should work.
https://webidl.spec.whatwg.org/#SameObject
The [SameObject] extended attribute must not be used on anything other than a read only attribute whose type is an interface type or object.
Oh, maybe not 🙁.
Yeah, it looks like DOMString is just a type, not an interface type: https://www.w3.org/TR/DOM-Level-3-Core/core.html#DOMString
In that case, I can add a prose saying that this property is cached upon update and the user code will get the same object for reuse after caching.
Such prose is a must even with [SameObject]
since it's just an indication and does not actually change the behavior. (https://github.com/whatwg/webidl/issues/212)
In other words, the actual algorithm should describe the caching mechanism.
@padenot Any thoughts? I can work on a PR based on https://github.com/WebAudio/web-audio-api/issues/2514#issuecomment-1268767325 and https://github.com/WebAudio/web-audio-api/issues/2514#issuecomment-1273870610.
Yes, that sounds good to me, thanks!
@saschanaz Is there any way to run autokagami against a PR? I would like to validate IDL before we land the change.
No direct way exists for now. We do have https://w3c.github.io/webidl2.js/checker/ where you can copy paste and check.
There also is w3c/spec-prod that does some validation in CI, but I don't think it does IDL validation right now. @sidvishnoi am I right?
@saschanaz spec-prod supports WebIDL validation since sometime. Powered by webidl2.js and reffy.
Yay! Then we can try replacing current autopublish.yml with spec-prod to take advantage of it 👍
@sidvishnoi Do you have an example GitHub action that I can take a look?
@hoch Here's a simplified example: https://w3c.github.io/spec-prod/#run-as-a-validator-on-pull-requests
If you want to see it working live: https://github.com/w3c/webappsec-permissions-policy/blob/main/.github/workflows/auto-publish.yml (this action is used by most W3C repos, so all examples are kinda same). https://github.com/w3c/webappsec-permissions-policy/actions/runs/3094677061/jobs/5008306993
Thank you both @saschanaz and @sidvishnoi for being so responsive. Super helpful. I'll add this action to our workflow.
Hmm. I am getting errors: https://github.com/WebAudio/web-audio-api/actions/runs/3229253879/jobs/5286332189#step:6:718
I guess I'll have to dig around the log and figure out why we couldn't put the validator in our workflow.
LINE 2822: Can't find the 'contextOptions' argument of method 'OfflineAudioContext/constructor(numberOfChannels, length, sampleRate)' in the argumentdef block. ✘ Did not generate, due to fatal errors
Just a validation error!
Turns out there were already several bikeshed errors that were suppressed: https://github.com/WebAudio/web-audio-api/actions/runs/3191055463/jobs/5206917845
Time to solve them 😉
This is now fixed by https://github.com/WebAudio/web-audio-api/pull/2517.
No comment
🤖 This is an automatic issue report for Web IDL validation error. 🤖
The bot found validation errors but couldn't fix them automatically, since there is no proper autofix for them. Please check the following messages and fix them.
Please file an issue at https://github.com/saschanaz/webidl-updater/issues/new if you think this is invalid or should be enhanced.