-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: implement user context subscriptions #3039
base: main
Are you sure you want to change the base?
Conversation
224ef9b
to
9f90bbb
Compare
53c7ba5
to
dde1a3b
Compare
dde1a3b
to
085ab17
Compare
} | ||
// user context subscription. | ||
if (subscription.userContextIds.size !== 0) { | ||
if (!contextId) { |
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.
nit: contextId
became opaque. Consider renaming to browsingContextId
.
@@ -69,6 +69,35 @@ async def test_subscribeWithoutContext_subscribesToEventsInAllContexts( | |||
assert resp["params"]["context"] == context_id | |||
|
|||
|
|||
@pytest.mark.asyncio | |||
async def test_subscribeUserContext(websocket, context_id, html): |
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.
it would be nice to have a test for a cusom user context as well. However we can delegate it to WPT tests, up to you
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.
in general LGTM, but I have a few minor comments
Issue #2983