Bug 202484 – Implement the Top-level await proposal

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Myles Borins]

The proposal is currently Stage-3 in the TC39 process which means it is ready for engines to implement.

https://github.com/tc39/proposal-top-level-await

I'm the champion from the TC39 committee so please feel free to ask me any questions you might have.

[Radar WebKit Bug Importer]

<rdar://problem/55999996>

[Keith Miller]

Created attachment 418936 [details]
WIP

[Keith Miller]

Created attachment 421151 [details]
WIP

[Keith Miller]

Created attachment 421154 [details]
Patch

[Keith Miller]

Created attachment 421155 [details]
Patch

[Keith Miller]

Created attachment 421156 [details]
Patch

[Keith Miller]

Created attachment 421157 [details]
Patch

[Keith Miller]

Created attachment 421158 [details]
Patch

[Yusuke Suzuki]

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?

[Keith Miller]

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.

[Keith Miller]

Created attachment 421159 [details]
Patch for landing

[Keith Miller]

Landed in r273225

[Aakash Jain]

Seems like there was a follow-up fix: https://trac.webkit.org/r273226 (mentioning here just for reference)

[Aakash Jain]

(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

[Keith Miller]

(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.

[Ryan Haddad]

(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