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
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.
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?I think this case is covered by https://www.chromium.org/blink/launching-features/#behavior-changes
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?
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?We have some Chromium tests that are checking for null, for instance: https://crsrc.org/c/third_party/blink/web_tests/fast/events/constructors/midi-connection-event-constructor.html;l=14It 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.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-api-owners-discuss/CA%2BuAeqSw_tdwQC4LCjPFtF3jPf8C7jEU_EfWVhGb_m3yvHUP4A%40mail.gmail.com.
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.
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.
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.
OK, thanks for clarifying. If you don't expect an actual behavior change, a PSA SGTM.
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
nullwhereas 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:1Is 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.
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?