| Summary: | Implement the Top-level await proposal | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Myles Borins <myles.borins> | ||||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Keith Miller <keith_miller> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Enhancement | CC: | aakash_jain, adieulot, agafvv, ap, benjamin, briankimpossible, calvaris, chi187, cyb.ai.815, ews-watchlist, fpizlo, hotaru, joepeck, keith_miller, mark.lam, msaboff, saam, tzagallo, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf, ysuzuki | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||
| OS: | All | ||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=222453 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Myles Borins
2019-10-02 11:09:36 PDT
Created attachment 418936 [details]
WIP
Created attachment 421151 [details]
WIP
Created attachment 421154 [details]
Patch
Created attachment 421155 [details]
Patch
Created attachment 421156 [details]
Patch
Created attachment 421157 [details]
Patch
Created attachment 421158 [details]
Patch
Comment on attachment 421156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421156&action=review Please check WebCore side too since now Workers support modules? > Source/JavaScriptCore/API/JSAPIGlobalObject.mm:261 > + // FIXME: We should update the delegate callbacks for async modules. Can you file a bug and paste URL? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:958 > + for (unsigned i = 0; i <= JSGenerator::Argument::NumberOfArguments; ++i) Why isn't it `< jsgenerator::Argument::NumberOfArguments`? If it is because |this| parameter, I think, initializeNextParameter(); // |this| for (unsigned i = 0; i < JSGenerator::Argument::NumberOfArguments; ++i) initializeNextParameter(); would be better. > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3747 > + // RELEASE_ASSERT(m_scopeNode->isFunctionNode()); Why is it necessary? If this comment-out is right, can you remove this assert? > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5066 > + // FIXME: It seems like this will create a lot of constants if there are many yield points. Maybe we should op_inc the old state. Can you file a bug and paste URL? Comment on attachment 421156 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421156&action=review >> Source/JavaScriptCore/API/JSAPIGlobalObject.mm:261 >> + // FIXME: We should update the delegate callbacks for async modules. > > Can you file a bug and paste URL? Done. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:958 >> + for (unsigned i = 0; i <= JSGenerator::Argument::NumberOfArguments; ++i) > > Why isn't it `< jsgenerator::Argument::NumberOfArguments`? If it is because |this| parameter, I think, > > initializeNextParameter(); // |this| > for (unsigned i = 0; i < JSGenerator::Argument::NumberOfArguments; ++i) > initializeNextParameter(); > > would be better. Yeah, that's the reason. I'll change. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3747 >> + // RELEASE_ASSERT(m_scopeNode->isFunctionNode()); > > Why is it necessary? If this comment-out is right, can you remove this assert? Yeah missed that during cleanup. Deleted. >> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:5066 >> + // FIXME: It seems like this will create a lot of constants if there are many yield points. Maybe we should op_inc the old state. > > Can you file a bug and paste URL? Done. Created attachment 421159 [details]
Patch for landing
Landed in r273225 Seems like there was a follow-up fix: https://trac.webkit.org/r273226 (mentioning here just for reference) (In reply to Keith Miller from comment #12) > Landed in r273225 This broke 5 layout-tests on mac and ios, as also indicated by ews. Should have waited for EWS to complete. - imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/module/evaluation-order-4-tla.html - imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-delayed.html - imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-import.html - imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-promise.html - imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/document-write/module-tla-immediate-promise.html (In reply to Aakash Jain from comment #14) > (In reply to Keith Miller from comment #12) > > Landed in r273225 > This broke 5 layout-tests on mac and ios, as also indicated by ews. Should > have waited for EWS to complete. > > - > imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script- > element/module/evaluation-order-4-tla.html > - > imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/ > document-write/module-tla-delayed.html > - > imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/ > document-write/module-tla-import.html > - > imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/ > document-write/module-tla-promise.html > - > imported/w3c/web-platform-tests/html/webappapis/dynamic-markup-insertion/ > document-write/module-tla-immediate-promise.html Weird that those didn't show up for me locally. They just need to be rebaselined. (In reply to Aakash Jain from comment #14) > (In reply to Keith Miller from comment #12) > > Landed in r273225 > This broke 5 layout-tests on mac and ios, as also indicated by ews. Should > have waited for EWS to complete. Rebaselined these tests in r273261 |