Groups keyboard shortcuts have been updated
Dismiss
See shortcuts

PSA / Discussion: MIDIConnectionEvent and MIDIMessageEvent nullable attributes

463 views
Skip to first unread message

Michael Wilson

unread,
Oct 18, 2023, 1:38:38 AM10/18/23
to blink-api-ow...@chromium.org
Hello,

I would like to merge the following change, and we're not sure if this needs to go through the launch process or if an announcement would be enough:


It is changing the nullability of some attributes in the IDL to align with a change in the specification, and Chromium already returns null for these elements under some circumstances.  Is there a precedent for how to handle this kind of change?

Best,
Michael Wilson

Yoav Weiss

unread,
Oct 18, 2023, 3:26:57 AM10/18/23
to Michael Wilson, blink-api-ow...@chromium.org
Hey Michael!

On Wed, Oct 18, 2023 at 7:38 AM Michael Wilson <mjwi...@chromium.org> wrote:
Hello,

I would like to merge the following change, and we're not sure if this needs to go through the launch process or if an announcement would be enough:


It is changing the nullability of some attributes in the IDL to align with a change in the specification, and Chromium already returns null for these elements under some circumstances.

So you're expecting code that doesn't properly handle these attributes as nulls to already sometimes fail in Chromium?
 
  Is there a precedent for how to handle this kind of change?



Best,
Michael Wilson

--
You received this message because you are subscribed to the Google Groups "blink-api-owners-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-api-owners-d...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-api-owners-discuss/CA%2BuAeqRMxR-mbzgrguZjLas0yzXbR1qMUNCbY-KMnwYR7p1-wQ%40mail.gmail.com.

Michael Wilson

unread,
Oct 18, 2023, 12:55:10 PM10/18/23
to Yoav Weiss, blink-api-ow...@chromium.org
Hi Yoav!

So you're expecting code that doesn't properly handle these attributes as nulls to already sometimes fail in Chromium?

Yes.

One thing I don't understand is how Chromium is able to return null now if the IDL doesn't have the attributes marked as nullable.  Is that something we don't automatically enforce, or does it indicate that this might need to be a larger change?


It looks like there are probably not many existing applications that use the API in a way that could result in null besides tests, polyfills, or similar (see https://github.com/WebAudio/web-midi-api/pull/252#issuecomment-1746495431).

 
  Is there a precedent for how to handle this kind of change?

Thank you.  Looking through the criteria in Step 2, I think this should be a PSA:
- The change is making the IDL match Chromium's existing behavior, so there shouldn't be a possibility of breakage
- Mozilla is in full agreement

Then, I think my next steps would be:
- Make a ChromeStatus entry
- Send a PSA
- Wait a bit for feedback
- Merge the change if there are no objections

I don't think there is a way to flag guard an IDL change, so I will not plan to flag guard this change.

Does that cover everything?

Best,
Michael

David Baron

unread,
Oct 18, 2023, 10:06:10 PM10/18/23
to Michael Wilson, Yoav Weiss, blink-api-ow...@chromium.org
On Wed, Oct 18, 2023 at 9:55 AM Michael Wilson <mjwi...@chromium.org> wrote:
One thing I don't understand is how Chromium is able to return null now if the IDL doesn't have the attributes marked as nullable.  Is that something we don't automatically enforce, or does it indicate that this might need to be a larger change?

Maybe a little bit off-topic for blink-api-owners-discuss at this point -- but:

We currently do very little (if any) enforcement that the return type (or return value) in C++ matches the return type that would be implied by WebIDL.  This was bothering me and I did some work to improve the situation in crbug.com/1464089.  (Though the main CL there isn't landed yet because I've been postponing addressing the review comments in favor of higher priority work, but hope to do it soon.)  That said, compile-time checks (such as those I'm adding in that CL) wouldn't catch this particular case since this is one of the cases where the nullable variant of the type has the same C++ type as the base type (since the base type corresponds to a pointer type in C++).  We could potentially add runtime CHECK()s or DCHECK()s to enforce this, but I haven't thought about that much, or talked to the owners of the bindings code about it.

-David

Yoav Weiss

unread,
Oct 19, 2023, 1:59:18 AM10/19/23
to Michael Wilson, blink-api-ow...@chromium.org
On Wed, Oct 18, 2023 at 6:45 PM Michael Wilson <mjwi...@chromium.org> wrote:
Hi Yoav!

So you're expecting code that doesn't properly handle these attributes as nulls to already sometimes fail in Chromium?

Yes.

One thing I don't understand is how Chromium is able to return null now if the IDL doesn't have the attributes marked as nullable.  Is that something we don't automatically enforce, or does it indicate that this might need to be a larger change?


It looks like there are probably not many existing applications that use the API in a way that could result in null besides tests, polyfills, or similar (see https://github.com/WebAudio/web-midi-api/pull/252#issuecomment-1746495431).

If returned nulls are not currently a common occurrence, is it possible that web developers aren't handling this well and simply haven't ran into it until this change? 
Given that there is some compat discussion to be had here, I think it'd be better to err on the side of sending an intent.

Michael Wilson

unread,
Oct 19, 2023, 1:37:10 PM10/19/23
to Yoav Weiss, blink-api-ow...@chromium.org
On Wed, Oct 18, 2023 at 10:59 PM Yoav Weiss <yoav...@google.com> wrote:
If returned nulls are not currently a common occurrence, is it possible that web developers aren't handling this well and simply haven't ran into it until this change? 

I don't think so.

This proposal is just to update the IDL interface to match Chromium's current behavior.  Chromium's current behavior will not be changed.  Thus, any problems with handling null attribute values should be the same before and after this change.

My understanding is that the attributes will only be null if they are initialized to null or if the optional initializer is omitted.  They cannot start off as non-null and later become null.  I think this further reduces potential confusion.

Given that there is some compat discussion to be had here, I think it'd be better to err on the side of sending an intent.

If you still think so given the above, I will do so.  I now see how to flag guard IDL files from this page: https://chromium.googlesource.com/chromium/src/+/main/third_party/blink/renderer/platform/RuntimeEnabledFeatures.md so I think I have all the pieces now to do either an intent or a PSA!

Best,
Michael

David Baron

unread,
Oct 19, 2023, 4:52:00 PM10/19/23
to Michael Wilson, Yoav Weiss, blink-api-ow...@chromium.org
For what it's worth, I think (if my understanding of the WebIDL behavior is correct) the interface changes in https://chromium-review.googlesource.com/c/chromium/src/+/4917021/3 don't cause any web-exposed behavior change, but I think the dictionary changes do cause a small web-exposed behavior change since they will cause exceptions to be thrown when explicit null values are used for the relevant values in those dictionaries.

-David

Michael Wilson

unread,
Oct 19, 2023, 5:10:12 PM10/19/23
to David Baron, Yoav Weiss, blink-api-ow...@chromium.org
I am not an IDL expert, but based on this comment the nullability is implied in a dictionary member:

Is that correct?  The IDL spec seems to fall through past step 4.5 if the member is not explicitly required:

I will test it to see what Chromium's behavior is.

Best,
Michael

Michael Wilson

unread,
Oct 19, 2023, 6:40:58 PM10/19/23
to David Baron, Yoav Weiss, blink-api-ow...@chromium.org
Hi David,

On Thu, Oct 19, 2023 at 1:52 PM David Baron <dba...@chromium.org> wrote:
For what it's worth, I think (if my understanding of the WebIDL behavior is correct) the interface changes in https://chromium-review.googlesource.com/c/chromium/src/+/4917021/3 don't cause any web-exposed behavior change, but I think the dictionary changes do cause a small web-exposed behavior change since they will cause exceptions to be thrown when explicit null values are used for the relevant values in those dictionaries.

I just checked and you are correct.  Before the change we get the following:

> new MIDIMessageEvent("something", {data: null}).data
null

> new MIDIConnectionEvent("something", {port: null}).port
null

whereas after the change we get the following:

> new MIDIMessageEvent("something", {data: null}).data
VM261:1 Uncaught TypeError: Failed to construct 'MIDIMessageEvent': Failed to read the 'data' property from 'MIDIMessageEventInit': Failed to convert value to 'Uint8Array'.
    at <anonymous>:1:1

> new MIDIConnectionEvent("something", {port: null}).port
VM285:1 Uncaught TypeError: Failed to construct 'MIDIConnectionEvent': Failed to read the 'port' property from 'MIDIConnectionEventInit': Failed to convert value to 'MIDIPort'.
    at <anonymous>:1:1

I believe the Working Group's intention was to align behavior with existing Chromium behavior, so I will bring this up on the spec repository and see if we can make the dictionary members explicitly nullable as well.

Thank you all for the discussion, and I will post again when I have a conclusion from the Audio Working Group.

Best,
Michael

Yoav Weiss

unread,
Oct 20, 2023, 2:04:50 AM10/20/23
to Michael Wilson, blink-api-ow...@chromium.org
On Thu, Oct 19, 2023 at 7:37 PM Michael Wilson <mjwi...@chromium.org> wrote:
On Wed, Oct 18, 2023 at 10:59 PM Yoav Weiss <yoav...@google.com> wrote:
If returned nulls are not currently a common occurrence, is it possible that web developers aren't handling this well and simply haven't ran into it until this change? 

I don't think so.

This proposal is just to update the IDL interface to match Chromium's current behavior.  Chromium's current behavior will not be changed.  Thus, any problems with handling null attribute values should be the same before and after this change.

My understanding is that the attributes will only be null if they are initialized to null or if the optional initializer is omitted.  They cannot start off as non-null and later become null.  I think this further reduces potential confusion.

OK, thanks for clarifying. If you don't expect an actual behavior change, a PSA SGTM.

Michael Wilson

unread,
Oct 20, 2023, 11:56:26 AM10/20/23
to Yoav Weiss, blink-api-ow...@chromium.org
On Thu, Oct 19, 2023 at 7:35 PM Domenic Denicola <dom...@chromium.org> wrote:
Stated another way, omitting the `required` keyword is the correct way to make dictionary members optional. Making them nullable is not the right way to do this.

Got it, we'll keep the current API shape then (nullable attributes on the interface, omitted `required` keyword on dictionary members). 

On Thu, Oct 19, 2023 at 11:04 PM Yoav Weiss <yoav...@google.com> wrote:
OK, thanks for clarifying. If you don't expect an actual behavior change, a PSA SGTM.

It looks like there will be a behavior change for explicitly initializing the dictionary members with null; to reiterate:

Before the change we get the following:

> new MIDIMessageEvent("something", {data: null}).data
null

> new MIDIConnectionEvent("something", {port: null}).port
null

whereas after the change we get the following:

> new MIDIMessageEvent("something", {data: null}).data
VM261:1 Uncaught TypeError: Failed to construct 'MIDIMessageEvent': Failed to read the 'data' property from 'MIDIMessageEventInit': Failed to convert value to 'Uint8Array'.
    at <anonymous>:1:1

> new MIDIConnectionEvent("something", {port: null}).port
VM285:1 Uncaught TypeError: Failed to construct 'MIDIConnectionEvent': Failed to read the 'port' property from 'MIDIConnectionEventInit': Failed to convert value to 'MIDIPort'.
    at <anonymous>:1:1

Is this enough to require an intent?  I think it's an unusual usage but it would cause breakage if there's anybody doing it.  There shouldn't be any other behavior change.

Best,
Michael
 

Yoav Weiss

unread,
Oct 23, 2023, 1:42:20 AM10/23/23
to Michael Wilson, blink-api-ow...@chromium.org
On Fri, Oct 20, 2023 at 5:56 PM Michael Wilson <mjwi...@chromium.org> wrote:
On Thu, Oct 19, 2023 at 7:35 PM Domenic Denicola <dom...@chromium.org> wrote:
Stated another way, omitting the `required` keyword is the correct way to make dictionary members optional. Making them nullable is not the right way to do this.

Got it, we'll keep the current API shape then (nullable attributes on the interface, omitted `required` keyword on dictionary members). 

On Thu, Oct 19, 2023 at 11:04 PM Yoav Weiss <yoav...@google.com> wrote:
OK, thanks for clarifying. If you don't expect an actual behavior change, a PSA SGTM.

It looks like there will be a behavior change for explicitly initializing the dictionary members with null; to reiterate:

Before the change we get the following:

> new MIDIMessageEvent("something", {data: null}).data
null

> new MIDIConnectionEvent("something", {port: null}).port
null

whereas after the change we get the following:

> new MIDIMessageEvent("something", {data: null}).data
VM261:1 Uncaught TypeError: Failed to construct 'MIDIMessageEvent': Failed to read the 'data' property from 'MIDIMessageEventInit': Failed to convert value to 'Uint8Array'.
    at <anonymous>:1:1

> new MIDIConnectionEvent("something", {port: null}).port
VM285:1 Uncaught TypeError: Failed to construct 'MIDIConnectionEvent': Failed to read the 'port' property from 'MIDIConnectionEventInit': Failed to convert value to 'MIDIPort'.
    at <anonymous>:1:1

Is this enough to require an intent?  I think it's an unusual usage but it would cause breakage if there's anybody doing it.  There shouldn't be any other behavior change.

If we're starting to throw in such cases, an intent is needed. It'd be best to also come to it with some data (HTTPArchive analysis, or usecounters) showing this doesn't happen.

Michael Wilson

unread,
Jan 6, 2025, 6:13:33 PMJan 6
to Yoav Weiss, blink-api-ow...@chromium.org
Hello all,

Digging this back up after a long period of being pre-empted by other work.

The midi_message_event interface was actually already fixed by a different change here:

So what I would like to do is rework https://crrev.com/c/4917021 to just add nullability to port in midi_connection_event, which should not cause any web-exposed behavior change, and file another issue for the dictionary changes (which would require an intent to ship, and I would look into later).

Does that sound good?  Do we need a PSA in that case, since there would be no web-exposed change, or can I land it as a bugfix?

Best,
Michael

Yoav Weiss (@Shopify)

unread,
Jan 7, 2025, 4:30:32 AMJan 7
to Michael Wilson, Yoav Weiss, blink-api-ow...@chromium.org
On Tue, Jan 7, 2025 at 12:13 AM Michael Wilson <mjwi...@chromium.org> wrote:
Hello all,

Digging this back up after a long period of being pre-empted by other work.

The midi_message_event interface was actually already fixed by a different change here:

So what I would like to do is rework https://crrev.com/c/4917021 to just add nullability to port in midi_connection_event, which should not cause any web-exposed behavior change, and file another issue for the dictionary changes (which would require an intent to ship, and I would look into later).

Does that sound good?  Do we need a PSA in that case, since there would be no web-exposed change, or can I land it as a bugfix?

The not-web-exposed parts can just land as a bugfix.
 
Reply all
Reply to author
Forward
0 new messages