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

[Web] Ensure editor crossorigin isolation headers #98901

Conversation

adamscott
Copy link
Member

Follow-up to #86089.

An error prevents the registration of the service worker for the editor Web build.

Capture d’écran, le 2024-11-06 à 13 23 15

This PR ensures that ___GODOT_ENSURE_CROSSORIGIN_ISOLATION_HEADERS___ is being replaced in the service worker for the editor build.

@adamscott adamscott added this to the 4.4 milestone Nov 6, 2024
@adamscott adamscott requested a review from a team as a code owner November 6, 2024 18:25
@adamscott adamscott requested a review from a team November 6, 2024 18:25
platform/web/detect.py Outdated Show resolved Hide resolved
platform/web/detect.py Outdated Show resolved Hide resolved
@akien-mga
Copy link
Member

Code seems ok to me, but adding this kind of build option seems really niche. I don't fully understand who would use this option and when?

@adamscott
Copy link
Member Author

Code seems ok to me, but adding this kind of build option seems really niche. I don't fully understand who would use this option and when?

The issue right now is that the editor build is somewhat broken. And I don't like the idea of enforcing COI headers for editor builds.

@akien-mga
Copy link
Member

Code seems ok to me, but adding this kind of build option seems really niche. I don't fully understand who would use this option and when?

The issue right now is that the editor build is somewhat broken. And I don't like the idea of enforcing COI headers for editor builds.

Why do we need a compile time option for this instead of just performing this change if env.editor_build is True?

@adamscott
Copy link
Member Author

You're right. The SW even checks for the headers before applying them, so if the server is already setup, no cry no foul.

@adamscott adamscott force-pushed the add-editor-ensure-crossorigin-isolation-headers branch from ea49178 to 1a082fd Compare November 21, 2024 15:29
@adamscott adamscott changed the title [Web] Add new build variable for ensuring editor crossorigin isolation headers [Web] Ensure editor crossorigin isolation headers Nov 21, 2024
@adamscott
Copy link
Member Author

@akien-mga I simplified the PR, I enforce the placeholder to be true, without any build variable.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Looks good to me. The rationale makes sense to me as threads are a hard requirement for the web editor.

Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

LGTM

@Repiteo Repiteo merged commit 3a5ce2f into godotengine:master Nov 27, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 27, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants