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 support data for Chrome Android #17908

Closed
wants to merge 4 commits into from

Conversation

jugglinmike
Copy link
Contributor

Summary

The changes in the first commit were generated procedurally by the mdn-bcd-collector project using data collected from Chrome for Android and following three corrections to its interpretation of the "mirror" support value:

foolip/mdn-bcd-collector#2326
foolip/mdn-bcd-collector#2297
foolip/mdn-bcd-collector#2280

Test results and supporting details

The modifications in the second commit were made manually.

The Chromium commit "Rename XR interface to XRSystem" was released in version 83:

https://chromiumdash.appspot.com/commit/d7a0292ca39f93d358b68ee703b0eb4fb4901022

The Chromium commit "Enable drag and drop on Android platform by default" was released in version 54:

https://chromereleases.googleblog.com/2016/10/chrome-for-android-update.html

Related issues

The modifications in the third commit were made manually.

The mdn-bcd-collector tool has a deficiency which cause it to produce incorrect changes for the Notification constructor. See "Testing constructors with no arguments misses some ways of disabling constructors": foolip/mdn-bcd-collector#970

These changes were generated procedurally by the mdn-bcd-collector
project using data collected from Chrome for Android and following three
corrections to its interpretation of the "mirror" support value:

foolip/mdn-bcd-collector#2326
foolip/mdn-bcd-collector#2297
foolip/mdn-bcd-collector#2280
The commit "Rename XR interface to XRSystem" was released in version 83:

https://chromiumdash.appspot.com/commit/d7a0292ca39f93d358b68ee703b0eb4fb4901022

The commit "Enable drag and drop on Android platform by default" was
released in version 54:

https://chromereleases.googleblog.com/2016/10/chrome-for-android-update.html
The mdn-bcd-collector tool has a deficiency which cause it to produce
incorrect changes for the Notification constructor. See "Testing
constructors with no arguments misses some ways of disabling
constructors": foolip/mdn-bcd-collector#970
@github-actions github-actions bot added the data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Sep 28, 2022
@jugglinmike
Copy link
Contributor Author

@foolip @queengooborg This patch has been rejected in CI due to 15 instances where a sub-feature is labeled as "supported" while its parent is not. For instance:

✖ api.AudioProcessingEvent - Error → No support in chrome_android, but support is declared in the following sub-feature(s):
       → api.AudioProcessingEvent.AudioProcessingEvent: 57

However, that change is consistent with the data in mdn-bcd-results:

$ python -m json.tool < 6.2.2-chrome-android-105.0.0.0-android-10-834146e591.json | grep 'AudioProcessingEvent"' -C2
            {
                "exposure": "Window",
                "name": "api.AudioProcessingEvent.AudioProcessingEvent",
                "result": true
            },
--
            {
                "exposure": "Window",
                "name": "api.AudioProcessingEvent",
                "result": false
            },

This makes me suspect that there may be a problem with the tests themselves. Reviewing the code generated by mdn-bcd-collector, it looks like the test for api.AudioProcessingEvent is more strict than the test for api.AudioProcessingEvent.AudioProcessingEvent. Aren't they testing the same thing, though?

@foolip
Copy link
Contributor

foolip commented Oct 3, 2022

This makes me suspect that there may be a problem with the tests themselves. Reviewing the code generated by mdn-bcd-collector, it looks like the test for api.AudioProcessingEvent is more strict than the test for api.AudioProcessingEvent.AudioProcessingEvent. Aren't they testing the same thing, though?

This sort of thing happens when there are custom tests that aren't quite right. Looking at https://mdn-bcd-collector.appspot.com/tests/api/AudioProcessingEvent it seems like the problem is that the AudioProcessingEvent has more required arguments than the custom test provides.

@@ -49,7 +49,9 @@
"chrome": {
"version_added": "56"
},
"chrome_android": "mirror",
"chrome_android": {
"version_added": false
Copy link
Contributor

Choose a reason for hiding this comment

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

The parent feature has flag data, so I think this might be a case of #6509. In one way of interpreting the previous data it means that the AmbientLightSensor constructor was supported in Chrome Android 56, behind a flag. (The other interpretation would say the data is inconsistent.)

@jugglinmike
Copy link
Contributor Author

This sort of thing happens when there are custom tests that aren't quite right. Looking at https://mdn-bcd-collector.appspot.com/tests/api/AudioProcessingEvent it seems like the problem is that the AudioProcessingEvent has more required arguments than the custom test provides.

Got it. Here's a patch to fix that.

@jugglinmike
Copy link
Contributor Author

The other errors concern the PermissionsPolicyViolationReportBody and a few APIs in SpeechSynthesis. It seems like we'll need to address them all before we can land this. I'll start with the former for now:

✖ api.PermissionsPolicyViolationReportBody - Error → No support in chrome_android, but support is declared in the following sub-feature(s):
       → api.PermissionsPolicyViolationReportBody.message: 88
       → api.PermissionsPolicyViolationReportBody.toJSON: 88
✖ api.PermissionsPolicyViolationReportBody - Error → No support in oculus, but support is declared in the following sub-feature(s):
       → api.PermissionsPolicyViolationReportBody.message: 14.0
       → api.PermissionsPolicyViolationReportBody.toJSON: 14.0
✖ api.PermissionsPolicyViolationReportBody - Error → No support in opera_android, but support is declared in the following sub-feature(s):
       → api.PermissionsPolicyViolationReportBody.message: 63
       → api.PermissionsPolicyViolationReportBody.toJSON: 63
✖ api.PermissionsPolicyViolationReportBody - Error → No support in webview_android, but support is declared in the following sub-feature(s):
       → api.PermissionsPolicyViolationReportBody.message: 88
       → api.PermissionsPolicyViolationReportBody.toJSON: 88

The message and toJSON properties are indeed absent from the results gathered by the collector. I believe that's because they're not present in the IDL provided by w3c/webref, which is in alignment with the specification as it's published today.

It looks like MDN has good reason to document these, anyway: the message property was removed from the specification in 2018, and the toJSON property was proposed earlier this year (though I somehow suspect you were already aware of that).

Is this a case for a custom IDL file in mdn-bcd-collector?

@foolip
Copy link
Contributor

foolip commented Oct 7, 2022

Oh my, the incomplete Feature Policy → Permissions Policy rename again. cc @clelland

I added custom tests in foolip/mdn-bcd-collector#2406, but Chromium has [LegacyNoInterfaceObject] for the interface, so those tests will claim no support, and the collector will try to change the data.

@jugglinmike I'd suggest just backing out any changes to PermissionsPolicyViolationReportBody. There's still so much wrong in BCD that if you pull on all threads that need fixing, there's no end in sight. I'll usually file issues when I have to give up.

Due to known issues in the tooling, the automatically-generated changes
to `api/PermissionsPolicyViolationReportBody.json` were inaccurate and
needed to be manually removed.

mdn#17908 (comment)
@jugglinmike
Copy link
Contributor Author

Thanks, @foolip! I've manually reverted those changes as you suggested.

Moving on the final batch of errors, it appears that (similar to the AudioProcessingEvent) the mdn-bcd-collector project also has deficiencies in its custom tests for the Speech Synthesis API proposal. In this case, though, it looks like some normative changes might be in order.

@foolip
Copy link
Contributor

foolip commented Oct 14, 2022

@github-actions
Copy link

This pull request has merge conflicts that must be resolved before it can be merged.

@queengooborg
Copy link
Contributor

This PR is quite large and touches on many different interfaces. Could we split this off into smaller chunks for easier review and merging?

@clelland
Copy link

Re: PermissionsPolicyViolationReportBody; that shouldn't be listed as supported anywhere; it was in an origin trial (as FeaturePolicyViolationReportBody) between Chrome 73 and 75, but hasn't been shipped.

jugglinmike added a commit to bocoup/browser-compat-data that referenced this pull request Dec 2, 2022
Due to known issues in the tooling, the automatically-generated changes
to `api/PermissionsPolicyViolationReportBody.json` were inaccurate and
needed to be manually removed.

mdn#17908 (comment)
@jugglinmike
Copy link
Contributor Author

@jugglinmike jugglinmike closed this Dec 2, 2022
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