Skip to content
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

Update Chromium versions for BluetoothRemoteGATTService API #10197

Merged

Conversation

queengooborg
Copy link
Contributor

@queengooborg queengooborg commented May 2, 2021

This PR updates and corrects the real values for Chrome-based browsers for the BluetoothRemoteGATTService API, based upon results from the mdn-bcd-collector project (v3.0.1). Additionally, this simplifies the child data for the API, removing the redundant multiple statements and derived flag data. Results are manually confirmed for accuracy.

Tests Used: https://mdn-bcd-collector.appspot.com/tests/api/BluetoothRemoteGATTService

This PR updates and corrects the real values for Microsoft Edge for the `BluetoothRemoteGATTService` API, based upon results from the [mdn-bcd-collector](https://mdn-bcd-collector.appspot.com) project (v3.0.1).  Results are manually confirmed for accuracy.

Tests Used: https://mdn-bcd-collector.appspot.com/tests/api/BluetoothRemoteGATTService
@github-actions github-actions bot added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label May 2, 2021
@queengooborg queengooborg changed the title Update Edge versions for BluetoothRemoteGATTService API Update Chromium versions for BluetoothRemoteGATTService API May 14, 2021
Copy link
Contributor

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but I'd also like @ddbeck to take a look at the question of deduplicating.

Note that searching for "macOS only" finds a bunch more cases like this, and probably it's best to delete all of that in a single PR, separate from fixing the data?

@@ -125,27 +121,9 @@
"ie": {
"version_added": false
},
"opera": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with simplifying all of this repeated OS and flag information since it's already on the parent feature. But it does mean that information won't be visible on a page like https://developer.mozilla.org/en-US/docs/Web/API/BluetoothRemoteGATTService/getCharacteristic#browser_compatibility.

@ddbeck how do you think we should deal with cases like this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some disagreement on this point. See #6509 for the broader issue.

My approach to flags has been to only represent the flag on the feature that requires the flag, not its descendants (unless the flags differ). For example, a CSS property behind a flag doesn't need to duplicate the flag data on subfeatures for property values. This does lead to a poorer presentation for individual interface member pages, however. In those cases I don't think it's too bad: for SomeFlaggedInterface.method(), you'd get an informative SomeFlaggedInterface is not defined error.

But since there's no consensus, I'm happy to allow anything that looks reasonable. If you think it'd be preferable to have flags inherit, then go for it, but I'm not going to ask for it myself.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go along with removing the flags on subfeatures here. I don't know what I think should be the guideline :)

ddbeck
ddbeck previously requested changes May 19, 2021
@@ -125,27 +121,9 @@
"ie": {
"version_added": false
},
"opera": [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some disagreement on this point. See #6509 for the broader issue.

My approach to flags has been to only represent the flag on the feature that requires the flag, not its descendants (unless the flags differ). For example, a CSS property behind a flag doesn't need to duplicate the flag data on subfeatures for property values. This does lead to a poorer presentation for individual interface member pages, however. In those cases I don't think it's too bad: for SomeFlaggedInterface.method(), you'd get an informative SomeFlaggedInterface is not defined error.

But since there's no consensus, I'm happy to allow anything that looks reasonable. If you think it'd be preferable to have flags inherit, then go for it, but I'm not going to ask for it myself.

api/BluetoothRemoteGATTService.json Outdated Show resolved Hide resolved
@foolip
Copy link
Contributor

foolip commented May 24, 2021

The "macOS only.", "Linux and versions of Windows earlier than 10.", and "Windows 10." notes are very widespread, although inconsistently applied, and it would be great to change them all at once to not create confusion for future contributors trying to update parts of this data.

Some of the data also indicates unconditional support, like api.BluetoothRemoteGATTCharacteristic.writeValueWithResponse. It would be good to decide what level of OS support we should consider complete and consistently either drop notes or add them where missing.

@ddbeck
Copy link
Collaborator

ddbeck commented May 25, 2021

@foolip Yeah, I understand this stuff is widespread. We did adopt a guideline to try to encourage new data to be more consistent when it comes to OS limitations: https://github.com/mdn/browser-compat-data/blob/main/docs/data-guidelines.md#operating-system-limitations-imply-partial_implementation. We could revise it to be more assertive about what note is needed (e.g., "You must have a note that reads, …").

@foolip
Copy link
Contributor

foolip commented May 28, 2021

Right, so it should be partial_implementation in a lot of cases here, but when does it stop being partial_implementation? Chrome is supported on Windows, macOS, Linux and ChromeOS. The simple answer would be to say that it's partial_implementation if something isn't supported on any of those platforms. What feels a bit weird about that is that Linux support in particular won't matter much to web developers, since it's not a very significant part of the user share.

Copy link

@Edimalius Edimalius left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Spam]

@ddbeck
Copy link
Collaborator

ddbeck commented Jun 4, 2021

The simple answer would be to say that it's partial_implementation if something isn't supported on any of those platforms. What feels a bit weird about that is that Linux support in particular won't matter much to web developers, since it's not a very significant part of the user share.

My glib answer to that is: well, if a browser claims to support Linux, then excluding some features from Linux seems like a partial implementation to me.

But I get where you're coming from. I'd accept a revision of the guidelines to say something like, if it's not supported on either of the top two desktop operating systems (citing some source which names Windows and macOS), then the partial_implementation flag is required; otherwise, only the note is required.

@foolip
Copy link
Contributor

foolip commented Jun 8, 2021

My glib answer to that is: well, if a browser claims to support Linux, then excluding some features from Linux seems like a partial implementation to me.

Let's go with that simpler solution for now. @vinyldarkscratch do you feel like wrangling the Bluetooth into that shape? Alternatively, updating data in a way that matches the current structure and filing an issue about the Bluetooth data would be OK with me.

@queengooborg
Copy link
Contributor Author

This commit should do the trick?

@ddbeck ddbeck dismissed their stale review June 10, 2021 16:36

stale

@ddbeck ddbeck requested a review from foolip June 10, 2021 16:36
@queengooborg
Copy link
Contributor Author

Hey @foolip, could you give this one a re-review when you are able?

api/BluetoothRemoteGATTService.json Outdated Show resolved Hide resolved
@@ -125,27 +121,9 @@
"ie": {
"version_added": false
},
"opera": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go along with removing the flags on subfeatures here. I don't know what I think should be the guideline :)

@foolip foolip merged commit 2a6487a into mdn:main Jul 14, 2021
@queengooborg queengooborg deleted the api/BluetoothRemoteGATTService/edge-corrections branch July 14, 2021 21:36
@foolip
Copy link
Contributor

foolip commented Jul 14, 2021

I've filed #11532 about making the remaining data consistent with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants