Skip to content

LibWeb: ShadowRealms part 4 - enough for it to not crash#1993

Merged
ADKaster merged 5 commits intoLadybirdBrowser:masterfrom
shannonbooth:shadow-realm-mvp
Nov 5, 2024
Merged

LibWeb: ShadowRealms part 4 - enough for it to not crash#1993
ADKaster merged 5 commits intoLadybirdBrowser:masterfrom
shannonbooth:shadow-realm-mvp

Conversation

@shannonbooth
Copy link
Member

@shannonbooth shannonbooth commented Oct 27, 2024

Sitting ontop of #2134

This implements enough for the test case:

<script>
async function test() {
    const realm = new ShadowRealm()
    const aSync = realm.evaluate(`async () => 1 + 1`)
    aSync()
}

test();
</script>

To work.

Unfortunately, WPT test cases are not passing as there is something going wrong with importing scripts (meaning that the test harness is not set up correctly), but this is still progress forwards!

http://wpt.live/url/idlharness-shadowrealm.window.html s

@shannonbooth shannonbooth changed the title LibWeb: ShadowRealms part 3 - enough to get it mostly working LibWeb: ShadowRealms part 4 - enough to get it mostly working Oct 27, 2024
@shannonbooth shannonbooth force-pushed the shadow-realm-mvp branch 7 times, most recently from a16bd7c to 2b099fa Compare November 4, 2024 15:02
@shannonbooth shannonbooth changed the title LibWeb: ShadowRealms part 4 - enough to get it mostly working LibWeb: ShadowRealms part 4 - enough for it to not crash Nov 4, 2024
@shannonbooth shannonbooth marked this pull request as ready for review November 4, 2024 15:05
With the introduction of shadow realms, there will be two different
possible host defined objects. For clarity, rename the existing host
defined object to PrincipalHostDefined.
This class is the host defined field of a synthetic realm created as
part of a shadow realm.
This object represents the global object for a shadow realm. The IDL
generator will need to be adjusted to the '[Global]' extended attribute
and no '[Exposed]' field (the change in the test is not correct, as I
understand it), but this should be enough to get us started on
shadow realms.
This is enough for a basic shadow realm to work :^)

There is more that we still need to implement here such as module
loading and fixing up the global object, but this is enough to get some
basic usage working.

// 11. Perform ? SetDefaultGlobalBindings(realm).
set_default_global_bindings(realm);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should call ShadowRealmGlobalScopeGlobalMixin::initialize(realm, *this); here, as well as add_shadow_realm_exposed_interfaces(*this) (in a helper on the global scope/global object variable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

speaking of which, did the spec folks decide what things should be exposed to shadow realms? We'll need to go around sprinkling annotations all over our idl files for the idl generator to pick them up

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, spec folks have done so - those are all of the crashing IDL tests that we have currently. Here's a meta issue that shows what has been done: tc39/proposal-shadowrealm#393

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, not crashing after these changes, but still doesn't work :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah perfect, then it should be straightforward to add ShadowRealm as an option in GenerateWindowOrWorkerInterfaces ... which I guess should be renamed to GenerateGlobalExposedInterfaces or some such.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, yeah that's why I've left those other FIXME's you mentioned in the initialize steps. I need to implement that universal global scope and reshuffling some implementations from windoworworkerglobal scope mixin to a new universalglobalscope mixin

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha. well, given that, I think we're good to go on this part once CI is happy. Feel free to add more fixmes if you want though.

@ADKaster ADKaster merged commit 9598ed1 into LadybirdBrowser:master Nov 5, 2024
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