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

Defer new SW registration from a prerendered page until it's activated #131

Merged
merged 9 commits into from Feb 7, 2022

Conversation

noamr
Copy link
Collaborator

@noamr noamr commented Feb 2, 2022

This prevents nonsensical behavior such as a prerendering context creating a service-worker that can create notifications or cache in a way that affects non-prerendering contexts.

An attempt at #44

@noamr noamr changed the title Delay installation if all clients are prerendering Delay ServiceWorker installation if all clients are prerendering Feb 2, 2022
@noamr noamr changed the title Delay ServiceWorker installation if all clients are prerendering Discard prerendering browsing context if trying to register a new previously unregistered Service Worker Feb 2, 2022
@noamr noamr changed the title Discard prerendering browsing context if trying to register a new previously unregistered Service Worker Discard prerendering page registers a previously unregistered Service Worker Feb 2, 2022
@nyaxt
Copy link
Collaborator

nyaxt commented Feb 2, 2022

@nhiroki What do you think of this PR?

@noamr
Copy link
Collaborator Author

noamr commented Feb 2, 2022

Hmm I guess this makes less sense for the omnibox case, where the SW would usually be brand new. maybe it's better to just defer registration instead.

@noamr noamr changed the title Discard prerendering page registers a previously unregistered Service Worker Defer new SW registration from a prerendered page until it's activated Feb 2, 2022
@noamr
Copy link
Collaborator Author

noamr commented Feb 2, 2022

Hmm I guess this makes less sense for the omnibox case, where the SW would usually be brand new. maybe it's better to just defer registration instead.

Refactored to defer instead of discard. This is better for the user-agent initiated prerendering case, where otherwise any page with SW would not work.

@nhiroki
Copy link
Collaborator

nhiroki commented Feb 3, 2022

LGTM. Deferring service worker registration sounds good to me.

@noamr
Copy link
Collaborator Author

noamr commented Feb 3, 2022

LGTM. Deferring service worker registration sounds good to me.

Great! I think the way this PR works needs more baking, but I'll follow that path and also create the appropriate WPTs.

@noamr
Copy link
Collaborator Author

noamr commented Feb 3, 2022

@domenic, WDYT?

Copy link
Collaborator

@domenic domenic left a comment

Choose a reason for hiding this comment

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

LGTM with nit but I will continue discussion in the issue.

prerendering.bs Outdated Show resolved Hide resolved
Co-authored-by: Domenic Denicola <d@domenic.me>
@noamr
Copy link
Collaborator Author

noamr commented Feb 6, 2022

Can someone please merge?

@nhiroki
Copy link
Collaborator

nhiroki commented Feb 7, 2022

LGTM, but I don't have permission to merge.

@nyaxt
Copy link
Collaborator

nyaxt commented Feb 7, 2022

@jeremyroman Would you merge this PR?

@domenic domenic merged commit f48d177 into WICG:main Feb 7, 2022
@noamr noamr deleted the sw-defer-install branch February 7, 2022 16:58
@jeremyroman
Copy link
Collaborator

Sorry for not catching this when I was pinged; I've fallen behind on email. Added @nyaxt and @nhiroki to the merge ACL to remove this obstacle in the future.

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.

None yet

5 participants