Skip to content

Make MIDIMessageEvent data MIDIConnectionEvent port nullable #252

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

Merged
merged 4 commits into from
Oct 6, 2023

Conversation

mjwilson-google
Copy link
Contributor

This is to fix #168 and #233.

It seems like there are two possible approaches to fixing these issues:

  • One, the "correct" approach as done in this PR, is to make the constructor arguments non-optional and the fields in the dictionaries required. This is how we handle the similar pattern in Web Audio (for example, https://webaudio.github.io/web-audio-api/#OfflineAudioCompletionEvent).
  • Two, the "pragmatic" approach is to make the attributes nullable. This is considered pragmatic because we won't have to change Chromium if we do this.

While pragmatism is generally a good thing, I would like to consider this one more time with Gecko and Chromium stakeholders before merging a change.

If it's not a lot of work to update the implementations, and if we don't expect this to break applications, it seems like making the arguments required might be a better API. But, if we feel strongly that it's better to simply make this nullable to move forward more quickly I will modify this change to that form.

@mjwilson-google mjwilson-google added the Needs Discussion The issue needs more discussion before it can be fixed. label Oct 2, 2023
@mjwilson-google mjwilson-google added this to the CR milestone Oct 2, 2023
@mjwilson-google mjwilson-google self-assigned this Oct 2, 2023
@padenot
Copy link
Member

padenot commented Oct 3, 2023

Unfortunately we can't make it required now, we have two implementations that have shipped with it being optional.

This is the implementation in Gecko:
https://searchfox.org/mozilla-central/source/dom/midi/MIDIMessageEvent.cpp#66-75
https://searchfox.org/mozilla-central/source/dom/webidl/MIDIMessageEvent.webidl#24
https://searchfox.org/mozilla-central/source/dom/webidl/MIDIConnectionEvent.webidl#23

@chrisguttandin
Copy link

Wouldn't this only affect code that calls the event constructors? I mean something like this would be affected, right?

new MIDIConnectionEvent('just-a-type-but-no-options');
new MIDIMessageEvent('just-a-type-but-no-options');

I would argue that this is very rarely done if you are just using the Web MIDI API. It looks like at least on GitHub all code doing something like this is either an implementation of the spec, a polyfill, a platform test or something similar.

https://github.com/search?q=%22new+MIDIConnectionEvent%28%22&type=code
https://github.com/search?q=%22new+MIDIMessageEvent%28%22&type=code

In that case it's maybe still fine to change it.

@mjwilson-google
Copy link
Contributor Author

@chrisguttandin Thank you for the code links! Yes, it would affect the cases you noted. Even if it's mostly tests and polyfills now, since the API is being used in this way that is more motivation to take the "pragmatic" approach and codify the existing behavior instead of trying to change things.

@padenot Thank you for the Gecko standpoint. Blink has the same standpoint. I will update this PR to make the data and port nullable instead (probably will get to it early next week).

@mjwilson-google mjwilson-google marked this pull request as draft October 4, 2023 16:10
@mjwilson-google mjwilson-google removed the Needs Discussion The issue needs more discussion before it can be fixed. label Oct 5, 2023
@mjwilson-google mjwilson-google changed the title Make init dictionaries required Make MIDIMessageEvent data MIDIConnectionEvent port nullable Oct 5, 2023
@mjwilson-google
Copy link
Contributor Author

This makes the data and port nullable. Note that the dictionary elements are not explicitly marked nullable, as per discussion in #233. Both Gecko and Blink should update their IDL after this change.

@mjwilson-google mjwilson-google marked this pull request as ready for review October 5, 2023 20:50
@padenot
Copy link
Member

padenot commented Oct 6, 2023

https://bugzilla.mozilla.org/show_bug.cgi?id=1857420 is the Gecko side for this.

@mjwilson-google
Copy link
Contributor Author

Chromium CL here: https://crrev.com/c/4917021

@mjwilson-google mjwilson-google merged commit 5d8e193 into WebAudio:gh-pages Oct 6, 2023
@mjwilson-google mjwilson-google deleted the 168-require-init branch October 6, 2023 17:05
aarongable pushed a commit to chromium/chromium that referenced this pull request Jan 7, 2025
This aligns MIDIConnectionEvent with the following change to the
Web MIDI spec:
WebAudio/web-midi-api#252

MIDIMessageEvent was updated here: https://crrev.com/c/6044556

Changing the nullability of dictionaries could cause web compatibility
issues and requires an intent to ship.  Due to that, just update the
bug link for the dictionaries and do not remove the nullable mark in
this CL.

Bug: 40286113
Change-Id: I34c76be4181aa45f9d81fa7dda9da7ba61196fa2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4917021
Commit-Queue: Michael Wilson <[email protected]>
Reviewed-by: Hongchan Choi <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1403068}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIDIMessageEventInit and MIDIConnectionEventInit members should be required
3 participants