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

Set true values in API for Safari with version_added: null #3673

Closed
wants to merge 16 commits into from

Conversation

queengooborg
Copy link
Collaborator

@queengooborg queengooborg commented Mar 19, 2019

Reviews and suggestions are highly encouraged and welcomed, especially as this is an almost entirely automated process, which hardly takes runtime flags into account. See something that looks inaccurate? Let me know!

This is to help the efforts of 2019 KR by replacing the null values with boolean values where applicable. All true values were identified within an IDL file in the WebKit source. This does not replace any values set to a version number. This PR is intended to remove as many null values as possible, and lead towards producing "real" values down the line.

I wrote the following JavaScript and Python files to create a list of all the values set to null and check upon their implementation in the IDL files respectively: https://gist.github.com/vinyldarkscratch/db303f7e72a582d2bf89d84bc4d035e2

Stats before and after this PR (based upon v0.0.73):

browser real values true values null values
total (before) 55.87% 22.49% 21.64%
total (after) 55.91% (+0.04%) 25.19% (+2.70%) 18.90% (-2.74%)
safari (before) 49.77% 19.16% 31.07%
safari (after) 49.93% (+0.16%) 30.55% (+11.39%) 19.52% (-11.55%)
safari ios (before) 41.59% 20.64% 37.79%
safari ios (after) 41.74% (+0.15%) 30.89% (+10.25%) 27.37% (-10.42%)

@queengooborg queengooborg changed the title Set true values in API for Safari with version_added: null Set true/false values in API for Safari with version_added: null Mar 20, 2019
@Elchi3 Elchi3 added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Mar 20, 2019
@foolip
Copy link
Collaborator

foolip commented Mar 21, 2019

As with #3658 there will be errors here of at least these kinds:

  • things in WebKit's IDL which haven't shipped yet in Safari
  • things which are shipped in Safari but where the Web IDL in WebKit is named differently
  • interface mixins

@queengooborg
Copy link
Collaborator Author

queengooborg commented Mar 21, 2019

I don’t doubt it, there’s sure to be errors in precise implementation in Safari! I realized that, while seeing your comments on the other PRs, I had the latest version of the WebKit source, not the particular version that was deployed for Safari 12. I knew, however, that there would be certain instances that would have to be checked by hand — as such, I didn’t create any “false” values within this PR, at least not yet. That leaves only potential for false positives, luckily (which in my opinion, is far better to have than false negatives).

I’m always looking for review/a second pair of eyes. Any and all feedback is highly appreciated!

@foolip
Copy link
Collaborator

foolip commented Mar 22, 2019

Ah, can you update the description to say this is only setting true values?

Do you think it'd make sense to first create a PR with null→true that have been confirmed by running tests? That's hopefully a lot, and makes it easier to review what remains since those are more likely to be incorrect.

@foolip
Copy link
Collaborator

foolip commented Mar 22, 2019

@vinyldarkscratch since I already had the data and scripts I sent #3700 and humbly request your review of that. I'm going to try rebasing this PR on top of that to see what kinds of changes will remain to review.

api/AbsoluteOrientationSensor.json Outdated Show resolved Hide resolved
@queengooborg queengooborg changed the title Set true/false values in API for Safari with version_added: null Set true values in API for Safari with version_added: null Mar 22, 2019
@Elchi3
Copy link
Member

Elchi3 commented Mar 28, 2019

How much overlap is there between this PR and #3700? I guess if both PRs update a subset from null to true we could trust that subset with relatively high confidence.

@queengooborg
Copy link
Collaborator Author

It looks like #3700 doesn't have anything extra that this PR doesn't already have, and there appears to be 1575 changes across 244 files that are in this PR but not the other, primarily regarding the ANGLE/EXT/OES/WebGL, SVG, and RTC APIs. So, we can trust the accuracy of #3700.

@foolip
Copy link
Collaborator

foolip commented Mar 29, 2019

I've also confirmed that #3700 is a perfect subset of this by reverting the commit from that PR on top of this PRs branch. That would have resulted in conflicts if #3700 contained changes that this does not.

Doing that leaves only 181 files with changes in this PR, which will be less to deal with.

@Elchi3
Copy link
Member

Elchi3 commented Mar 29, 2019

Okay, given this, I think we can merge #3700.
We have two independent approaches and I think that is enough for approving null -> true changes.

@Elchi3
Copy link
Member

Elchi3 commented Mar 29, 2019

Merged #3700 and there are indeed no merge conflicts. 👍

Can you please rebase this PR, so the changeset becomes smaller?

@queengooborg
Copy link
Collaborator Author

Rebase complete!

api/AnimationEffect.json Outdated Show resolved Hide resolved
@ddbeck ddbeck requested a review from Elchi3 April 1, 2019 09:32
@Elchi3 Elchi3 added the bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes label Aug 14, 2019
@Elchi3 Elchi3 removed their request for review August 21, 2019 10:19
@Elchi3
Copy link
Member

Elchi3 commented Aug 21, 2019

This has been open for a long time now and basically the reason why we haven't merged it is still as described in #3658 (comment)

I'm closing this as it is unlikely to merged currently. It can be re-opened if we gain new confidence with this approach or want to experiment with it again.

@Elchi3 Elchi3 closed this Aug 21, 2019
@queengooborg queengooborg deleted the api/safari-null branch October 29, 2019 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bulk_update 📦 An update to a mass amount of data, or scripts/linters related to such changes 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