Skip to content

Conversation

@sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Nov 5, 2024

This change extracts a few helper function to make the JS event system smaller and more internally consistent.

Firstly, we now use a single storage location for the event data pointer used in event callback. This is allocated on demand in allocateEventStruct. We then use a new dispatchEvent all the many loction that previously duplicated the event dispatching logic.

In order to handler the 3 cases where we need to keep a copy of the last dispatched event we also have two new helpers: getCachedEvent, cacheEvent. These allocate separate copies of the last generated event, but only for the 3 event types where this is required.

@sbc100 sbc100 force-pushed the shared_events branch 5 times, most recently from 1420cb7 to 6a42389 Compare November 5, 2024 05:09
@sbc100 sbc100 requested a review from kripken November 5, 2024 17:23
@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 5, 2024

Sadly I don't think we have code size tests that cover library_html5 but I think this would be pretty good code size saving.

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 5, 2024

For test_html5_mouse_closure we have:

Before:
10562 out/test/test.js
19922 out/test/test.wasm

After:
10771 out/test/test.js
20141 out/test/test.wasm

So a slight regression sadly :(

This change extracts a few helper function to make the JS event system
smaller and more internally consistent.

Firstly, we now use a single storage location for the event data pointer
used in event callback.  This is allocated on demand in
`allocateEventStruct`.  We then use a new `dispatchEvent` all the many
loction that previously duplicated the event dispatching logic.

In order to handler the 3 cases where we need to keep a copy of the
last dispatched event we also have two new helpers: `getCachedEvent`,
`cacheEvent`.  These allocate separate copies of the last generated
event, but only for the 3 event types where this is required.
@kripken
Copy link
Member

kripken commented Nov 5, 2024

This looks like a rather large refactoring, and I am worried about our lack of testing on this UI code. Are you certain we have coverage of all the things changed here? If not, perhaps before this PR we can look into adding tests?

@sbc100
Copy link
Collaborator Author

sbc100 commented Nov 5, 2024

This looks like a rather large refactoring, and I am worried about our lack of testing on this UI code. Are you certain we have coverage of all the things changed here? If not, perhaps before this PR we can look into adding tests?

Agreed, I'll put this on ice until I get around to adding more testing.

@sbc100 sbc100 marked this pull request as draft November 5, 2024 22:56
@sbc100 sbc100 closed this Jan 3, 2025
@sbc100 sbc100 deleted the shared_events branch January 3, 2025 22:45
@sbc100 sbc100 restored the shared_events branch January 4, 2025 00:23
@sbc100 sbc100 reopened this Jan 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants