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

Implement "owners" in workers #621

Merged
merged 1 commit into from
Dec 19, 2023
Merged

Implement "owners" in workers #621

merged 1 commit into from
Dec 19, 2023

Conversation

jrandolf-2
Copy link
Contributor

@jrandolf-2 jrandolf-2 commented Dec 14, 2023

Every dedicated worker has an outside setting that it's created under. This info is needed to understand the tree structure of dedicated workers.


Preview | Diff

@jrandolf-2 jrandolf-2 requested a review from jgraham December 14, 2023 14:47
@jrandolf-2 jrandolf-2 force-pushed the jrandolf/browsing-context branch from a806e34 to 701459e Compare December 14, 2023 14:52
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

This seems fine in principle, but I don't think the implementation works as written.

@jrandolf-2 jrandolf-2 force-pushed the jrandolf/browsing-context branch from 701459e to 70deeec Compare December 18, 2023 09:43
@jrandolf-2 jrandolf-2 changed the title Implement "parent" in dedicated workers Implement "owners" in dedicated workers Dec 18, 2023
@jrandolf-2 jrandolf-2 force-pushed the jrandolf/browsing-context branch 2 times, most recently from d4909a3 to baf59b7 Compare December 18, 2023 09:52
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

Any specific reason we're only doing this for dedicated workers? I think the same approach also works for shared workers.

@jrandolf-2 jrandolf-2 force-pushed the jrandolf/browsing-context branch 2 times, most recently from 763c992 to ec2da0e Compare December 18, 2023 10:08
@jrandolf-2
Copy link
Contributor Author

jrandolf-2 commented Dec 18, 2023

Any specific reason we're only doing this for dedicated workers? I think the same approach also works for shared workers.

@jgraham So I'm a little conflicted right now about exposing all owners. What I really want to expose is the "initial owner", not necessarily all of them. If we want to expose all of them, then we'd need events like realm.ownerAdded and realm.ownerRemoved to really capture the entire lifecycle of ownership.

What do you think about extending this to that length?

@jrandolf-2 jrandolf-2 requested a review from jgraham December 18, 2023 10:18
@jgraham
Copy link
Member

jgraham commented Dec 18, 2023

I think dedicated workers only ever have one owner, so for that case the events don't seem necessary. For shared workers it makes more sense to me to expose the owners in the same way we do for dedicated workers, but not add lifecycle events until there's clear user demand that it does to not expose the owners because we don't also have lifecycle events.

So, for now, I'd just add the field and information to all worker types, so that we at least have a consistent basis to build from, and consider extending the support later if necessary.

@jrandolf-2 jrandolf-2 force-pushed the jrandolf/browsing-context branch from ec2da0e to f5f87f8 Compare December 18, 2023 11:24
@jrandolf-2
Copy link
Contributor Author

I think dedicated workers only ever have one owner, so for that case the events don't seem necessary. For shared workers it makes more sense to me to expose the owners in the same way we do for dedicated workers, but not add lifecycle events until there's clear user demand that it does to not expose the owners because we don't also have lifecycle events.

So, for now, I'd just add the field and information to all worker types, so that we at least have a consistent basis to build from, and consider extending the support later if necessary.

Done.

@jrandolf-2 jrandolf-2 force-pushed the jrandolf/browsing-context branch 2 times, most recently from b57783e to 7d7e876 Compare December 18, 2023 11:33
@jrandolf-2 jrandolf-2 changed the title Implement "owners" in dedicated workers Implement "owners" in workers Dec 18, 2023
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

This basically looks fine, but I think there's a technical error copied from elsewhere in the spec.

@jrandolf-2 jrandolf-2 force-pushed the jrandolf/browsing-context branch from 7d7e876 to af0ceaf Compare December 19, 2023 11:54
@jrandolf-2 jrandolf-2 requested a review from jgraham December 19, 2023 11:54
@jrandolf-2 jrandolf-2 merged commit bb3f568 into main Dec 19, 2023
4 checks passed
@jrandolf-2 jrandolf-2 deleted the jrandolf/browsing-context branch December 19, 2023 12:02
github-actions bot added a commit that referenced this pull request Dec 19, 2023
SHA: bb3f568
Reason: push, by jrandolf

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants