Skip to content
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

Test base URL with multiple globals for WebSocket #39978

Merged
merged 3 commits into from
May 16, 2023

Conversation

zcorpan
Copy link
Member

@zcorpan zcorpan commented May 12, 2023

@zcorpan zcorpan requested a review from annevk May 12, 2023 09:14
annevk added a commit to whatwg/websockets that referenced this pull request May 12, 2023
And thereby relative URLs. They are instantly normalized to the ws: and wss: schemes.

Tests: web-platform-tests/wpt#39955 and web-platform-tests/wpt#39978.

Co-authored-by: Gus Caplan <me@gus.host>
Co-authored-by: Adam Rice <ricea@chromium.org>
window.scriptToRun = `
const result = {};
try {
const ws = new WebSocket('foo');
Copy link
Member

Choose a reason for hiding this comment

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

This ends up creating the WebSocket object in the incumbent realm I think, which then ends up being its relevant realm.

Copy link
Member

Choose a reason for hiding this comment

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

In general the way of testing these subtleties for constructor APIs like WebSocket or Worker are quite different than how you'd test them for methods like location.assign(). So I think following the pattern from https://github.com/web-platform-tests/wpt/tree/master/workers/multi-globals would work best.

Copy link
Member

Choose a reason for hiding this comment

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

This is not a great location for this file, since html/browsers/browsing-the-web/navigating-across-documents/ is supposed to be about the location.x() APIs.

Instead, it's probably best to create a new directory under websockets/, similar to how https://github.com/web-platform-tests/wpt/tree/master/workers/multi-globals is under workers/

window.scriptToRun = `
const result = {};
try {
const ws = new WebSocket('foo');
Copy link
Member

Choose a reason for hiding this comment

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

In general the way of testing these subtleties for constructor APIs like WebSocket or Worker are quite different than how you'd test them for methods like location.assign(). So I think following the pattern from https://github.com/web-platform-tests/wpt/tree/master/workers/multi-globals would work best.

@zcorpan
Copy link
Member Author

zcorpan commented May 15, 2023

Thanks, I've changed the test to be more like Worker. However, WebSocket spec says to use the relevant settings object, but going by the example in https://html.spec.whatwg.org/multipage/webappapis.html#realms-settings-objects-global-objects there's no way to test for the relevant settings object for a constructor (vs a method), since you can't use .call() with a constructor.

I used current instead, like Workers, but this needs a spec change. I'll file an issue. Edit: whatwg/websockets#46

@zcorpan zcorpan merged commit 9d415b8 into master May 16, 2023
@zcorpan zcorpan deleted the zcorpan/websocket-multiple-globals branch May 16, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants