Extract MainLoop out of Browser #12165
Conversation
Fix test_no_browser to check for MainLoop instead
curiousdannii
force-pushed
the
mainloop
branch
from
2ea57e3 to
ccf25eb
Compare
|
There are a few errors that I couldn't make sense of :/ |
|
Nice! Thanks for working on this. Can we maybe break this down into smaller changes so it more obvious if there are any actual changes here of it is NFC. To start with how about splitting out just the safetimers part? That seems like it might be separable. |
| var LibrarySafeTimers = { | ||
| $SafeTimers__postset: 'Module["requestAnimationFrame"] = function Module_requestAnimationFrame(func) { SafeTimers.requestAnimationFrame(func) };\n', | ||
| $SafeTimers: { | ||
| // abort and pause-aware versions TODO: build main loop on top of this? |
There was a problem hiding this comment.
Can we take this object's 7 member as make them into top level library members?
That way a program that only needs safeSetTimeout would depend only on that one functuion.
Of course each of the function would need to depend on allowAsyncCallbacks and queuedAsyncCallbacks so maybe that could be done with autoAddDeps?
There was a problem hiding this comment.
Good idea. Should they all be prefixed with emscripten_?
There was a problem hiding this comment.
No, these are library internal so should be prefixed with $.
Anything without a $ is visible as a C/C++ and we don't want that in this case I think.
The $ is only need in the __deps and on the decl itself.. When you call the function you don't need the $.
| @@ -0,0 +1,118 @@ | |||
| /** | |||
There was a problem hiding this comment.
I wonder if we can come up with a better name for this library? I'm not really sure what "safe timers" means. Do you know the history of this name or what it really means?
There was a problem hiding this comment.
I think they're "safe" in that the callbacks won't be run (immediately) if it's paused, or if the module has aborted. Though at present nothing actually calls the pause/resume functions. That functionality could be cut? (And readded later if it ever was needed.)
Sure, that makes sense. These changes shouldn't effect the public/published API, but if anyone has manually gone and looked up the source and added calls to the internal functions it would break. Would it be worth turning the old functions into wrappers of the new with assertion warnings? |
Instead, check if Browser exists or not, and add direct dependencies on Browser from the specific async wget methods that actually need full Browser support. Split out from #12165 Saves 13KB from a minified build.
For an internal API I think that's probably not needed. |
|
This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant. |
Fixes #5355
I just noticed that Asyncify mode requires MainLoop, but not because of anything inherent in Asyncify mode, but just because it needs to be aware of it. It would be great if Asyncify could handle MainLoop if it's included, but to not require it. Not too sure how to do that though. Trying a simple
typeof MainLoop !== 'undefined'Similarly,
MainLoop.safeSetTimeoutcould be split out into an even smaller library, so that if all you use isemscripten_sleepit wouldn't have to include all of MainLoop? Edit: This was very easy actually.