Web IDL validation errors

[autokagami]

🤖 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.

• Validation error at line 7 in webaudio,73, inside `interface AudioContext -> attribute sinkId`:
  			readonly attribute (DOMString or AudioSinkOptions) sinkId;
                                      ^
  

  Error: Attributes cannot accept dictionary types.

Please file an issue at https://github.com/saschanaz/webidl-updater/issues/new if you think this is invalid or should be enhanced.

[hoch]

@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!

[hoch]

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;
}

[padenot]

Yeah that would work.

[hoch]

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.

[hoch]

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.

[hoch]

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.

[alvinjiooo]

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.

[saschanaz]

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, setSinkId doesn't seem to clone the object. What happens if: Oh, actually scrap it. The IDL layer should automatically do that. It should still be somehow frozen to prevent modification if you go object way.

[hoch]

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?

[saschanaz]

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.)

[hoch]

Good point. With AudioSinkInfo option, do we need to enforce [SameObject] for the sinkId property?

[saschanaz]

Yup, that should work.

[saschanaz]

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 🙁.

[hoch]

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.

[saschanaz]

Such prose is a must even with [SameObject] since it's just an indication and does not actually change the behavior. (whatwg/webidl#212)

In other words, the actual algorithm should describe the caching mechanism.

[hoch]

@padenot Any thoughts? I can work on a PR based on #2514 (comment) and #2514 (comment).

[padenot]

Yes, that sounds good to me, thanks!

[hoch]

@saschanaz Is there any way to run autokagami against a PR? I would like to validate IDL before we land the change.

[saschanaz]

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?

[sidvishnoi]

@saschanaz spec-prod supports WebIDL validation since sometime. Powered by webidl2.js and reffy.

[saschanaz]

Yay! Then we can try replacing current autopublish.yml with spec-prod to take advantage of it 👍

[hoch]

@sidvishnoi Do you have an example GitHub action that I can take a look?

[sidvishnoi]

@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

[hoch]

Thank you both @saschanaz and @sidvishnoi for being so responsive. Super helpful. I'll add this action to our workflow.

[hoch]

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.

[saschanaz]

 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!

[saschanaz]

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 😉

[hoch]

This is now fixed by #2517.

[Kapileo1]

No comment