Skip to content

Test base URL with multiple globals for WebSocket#39978

Merged
zcorpan merged 3 commits intomasterfrom
zcorpan/websocket-multiple-globals
May 16, 2023
Merged

Test base URL with multiple globals for WebSocket#39978
zcorpan merged 3 commits intomasterfrom
zcorpan/websocket-multiple-globals

Conversation

@zcorpan
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

5 participants