-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose everywhere #209
Expose everywhere #209
Conversation
r? @domfarolino |
LGTM, though I'm wondering if we need to specify the side effects that happen in some of these other environments. |
On a first glance, I'm not seeing anything that obviously needs adjusting, but let me know if you disagree. |
@terinjokes @domfarolino can you sort out the CI failure? Seems like an image might need width/height attributes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding tests, you have:
TBD but quite trivial
This PR LGTM, but do you plan on adding tests?
@domfarolino For tests, I'll make sure to add an idlharness test as soon as the IDL changes propagate into the wpt repo after this PR is merged. I don't think there's much else I can test - maybe a manual test that the messages show up? |
A manual test would be nice I think |
Tests are up at web-platform-tests/wpt#34361 |
This and the tests LGTM. GitHub is complaining about the branch not being up to date, so if you could fix that then I think we'd be good to go. |
7bb6921
to
334b6f3
Compare
Done! |
Great, feel free to merge the tests whenever and I'll go ahead and merge this. |
Merged the wpt pr, thank you |
Ref: tc39/proposal-shadowrealm#331
(See WHATWG Working Mode: Changes for more details.)
Preview | Diff