Cleanup unused code, fix event handlers crash #4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MaxDesiatov
added
bug
j-f1
approved these changes
Dec 26, 2020
There was a problem hiding this comment.
LGTM for now but in the long term I’m worried about modifying the generated files too much. The specs we depend on will change from time to time and we should keep up to date. Ideally we’d have a way to list the APIs we’ve manually implemented in Swift, then tweak webidl2swift to avoid auto-generating those APIs.
|
Great point! I'm going to submit another PR to the generator soon to provide better tokenizer diagnostics, as I'm getting inscrutable errors with some other spec files. I hope I'm more familiar with the codebase by then to find a way to avoid overwriting manually updated files. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some weird names and typealiases were cleaned up (I don't think that
Int8ArrayOrInt16ArrayOrInt32ArrayOrUint8ArrayOrUint16ArrayOrUint32ArrayOrUint8ClampedArrayOrFloat32ArrayOrFloat64ArrayOrDataViewortypealias VoidFunction = (() -> Void)could be useful in any way).GlobalEventHandlersprotocol which was only inherited by a single concrete type was removed with its extension becoming the extension of the corresponding class.I've also cleaned up event handlers to retain and release closures when needed, which makes our single basic test pass in browsers. Basic CI config to verify that is added here.
A few other uses of
JSClosurewere updated accordingly to fix memory management. UnusedClosureHandlerproperty wrapper is removed, leaving onlyOptionalClosureHandler.