Set true/false values in API for Chromium with version_added: null

[queengooborg]

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 the 2019 KR by replacing the null values with boolean values where applicable. All true values were identified within an IDL file in the Chromium source, both by an automated script (see below), as well as manual source code review. False values were set for interfaces not found within the IDL files, and for interfaces not exposed in Webview. This does not remove any version numbers when trying to set them to true as to prevent the removal of real values. 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)	56.20% (+0.33%)	25.62% (+3.13%)	18.19% (-3.45%)
chrome (before)	66.15%	24.43%	9.42%
chrome (after)	67.06% (+0.91%)	27.49% (+3.06%)	5.45% (-3.97%)
chrome android (before)	51.11%	32.40%	16.49%
chrome android (after)	51.95% (+0.84%)	35.33% (+2.93%)	12.72% (-3.77%)
webview android (before)	48.09%	34.95%	16.96%
webview android (after)	48.89% (+0.80%)	37.48% (+2.53%)	13.63% (-3.33%)

[queengooborg]

@jpmedley I would love your review on this. Anything seem out of place to you? Let me know and I'll happily fix it!

[jpmedley]

This is terrific work. It gets rid of a log of bad data quickly.

Before proceeding, we need to deal with the scattering of situations where something is in Android, but not Android WebView.

The file below is from our test system and lists all items not supported in Webview.

https://cs.chromium.org/chromium/src/android_webview/tools/system_webview_shell/test/data/webexposed/not-webview-exposed.txt

If you want to sanity check, the easiest way is with the list of what's supposed to be there.

https://cs.chromium.org/chromium/src/android_webview/tools/system_webview_shell/test/data/webexposed/global-interface-listing-expected.txt?g=0

How hard is it to amend your script to check one of these files and put false in the appropriate interfaces?

[queengooborg]

💜

Since the script is based upon parsing various text files, and looking at the two files you've linked, this should be a breeze to incorporate them! I'll have it implemented with a new commit shortly. 😉

[queengooborg]

Looks like there was only a couple of files that had inaccurate data. They have now been fixed!

[jpmedley]

That is particularly awesome. I wonder if we might implement a lint test based on those files.

[foolip]

@vinyldarkscratch this is great, thanks for putting this together! There's overlap with #3689, so I'm going to cross-check to see if there are differences.

I see that you're parsing the Web IDL from sources, but aren't looking at RuntimeEnabled extended attributes and similar. This will lead to some false positives.

[foolip]

I've reviewed up to MediaQueryListListener.json, will finish reviewing after lunch.

[foolip]

Finished reviewing. There are plenty of edge cases, but with some changes reverted this will be a great improvement!

[foolip]

@vinyldarkscratch I cross-checked #3733 against this a bit and found lots of things that are in Chromium's IDL with no conditional exposure or other complications, but which still weren't updated by this PR. Something to look into?

[queengooborg]

@foolip wrote:

@vinyldarkscratch I cross-checked #3733 against this a bit and found lots of things that are in Chromium's IDL with no conditional exposure or other complications, but which still weren't updated by this PR. Something to look into?

The script I wrote to test for chrome: {version_added: null, ...} revealed no further results for the APIs on the current merge of master. It could be due to the fact that I may not be testing against arrays, but I doubt that a version_added: null will show up within a list. I know that other Chromium-based browsers may be null when Chrome is true -- it's why I've opened #3748. (This PR also doesn't convert false -> true.)

I'd love to look into it more! Can you inform me of a couple APIs and members that seem to fall within this scenario?

[foolip]

@vinyldarkscratch the 10 files I spot checked in that PR are in that category. First was Blob.

[queengooborg]

Okay, then yeah, #3748 handles all of those by also copying version numbers over. I thought about putting it into this PR, but I realized that this PR is already big as it is, so I separated it into its own.

[foolip]

@vinyldarkscratch this PR did update some webview_android data, are you sure that the missed interfaces aren't because of a bug in the script for this PR?

[queengooborg]

I’m certain — the input list I’ve given it was a list of all null values for Chrome Desktop, but the output changes all Chromium browsers, including Webview. What it doesn’t touch are the entries that already have Chrome Desktop support set to something other than null, which is where that other PR takes place.

Essentially:

• “chrome: {version_added: null}” — updates from WebIDL in this PR
• “chrome: {version_added: true/false/versionNumber}” — updates by cloning Chrome Desktop onto all other Chromium browsers in API: Mirror Chrome Desktop onto Chromium browsers when null #3748

Sorry, I think a part of the confusion may be my specific use of terms. When I refer to “Chromium”, I am talking about all Chrome-based browsers, rather than just Chrome Desktop. 😛

[foolip]

Sorry, I think a part of the confusion may be my specific use of terms. When I refer to “Chromium”, I am talking about all Chrome-based browsers, rather than just Chrome Desktop. stuck_out_tongue

That bit was very clear from the title PR/description, "Chromium-based browser" is the terminology I normally used for all of these together.

The surprise was that only entries that were originally null for chrome were updated.

[Elchi3]

@Elchi3 Do you want to review this before I merge?

Well, I could. Generally, I think I trust this approach less than the automation @foolip came up with. It is interpreting idl source code and on our way we've identified flaws in how parsing was done here. Mere existence in idl files doesn't always guarantee exposure as there can be flags, switches, conditions and what not. What do you think about this approach? Are confluence or other automation attempts verifying what is proposed in this PR? How did you approve this? I assume you didn't manually look into the 101 files here.

Finally, it would be good to attach a log to this PR what was changed for future reference. Like I did here #3899 or like Philip did here: #3734 (comment)

[queengooborg]

@jpmedley Your comments are in regards to a merge on master branch, not changes in this PR.

[Elchi3]

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.