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

spec.bs: Change how updating IGs to an unknown executionMode behaves. #927

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

MattMenke2
Copy link
Contributor

@MattMenke2 MattMenke2 commented Nov 22, 2023

When joining IGs with unrecognized modes, we use "compatibility" for forward compatibility. Seems like we should do the same for IG updates.

Note that neither the behavior this CL describes nor the originally described behavior are what currently happens - we currently ignore unrecognized modes. I want to get approval for this before changing behavior.


Preview | Diff

When joining IGs with unrecognized modes, we use "compatibility" for forward compatibility.  Seems like we should do the same for updates.

Note that neither this behavior nor the originally described behavior are what currently happens - we currently ignore unrecognized modes.  Want to get approval for this before changing behvaior.
@JensenPaul JensenPaul added the spec Relates to the spec label Nov 27, 2023
@qingxinwu
Copy link
Collaborator

qingxinwu commented Jan 25, 2024

It seems this PR's behaviour is what's in the implementation now (1 CL in Dec made the change which meets what is described here).
I have the same fix in https://github.com/WICG/turtledove/pull/1006/files#diff-6f5a1d8263b0b0c42e2716ba5750e3652e359532647ac934c1c70086ae3ceddaR3499. I'll merge this PR then, if you're ok with it

@qingxinwu qingxinwu requested a review from domfarolino February 6, 2024 16:32
@qingxinwu
Copy link
Collaborator

qingxinwu commented Feb 6, 2024

@domfarolino Mind taking a look at this tiny change? The code for this behaviour change landed last Dec (in the same CL that allowed updating exectionMode to "frozen-context", which got I2S approval and launched). So this spec change is a missing piece of that launch (that spec PR only mentioned the "frozen-context" part and forgot updating this)

Copy link
Collaborator

@domfarolino domfarolino left a comment

Choose a reason for hiding this comment

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

Yes this makes sense, thanks for describing the justification for this and how it aligns with the CL landed earlier!

@JensenPaul JensenPaul merged commit 2f6849b into WICG:main Feb 9, 2024
github-actions bot added a commit that referenced this pull request Feb 9, 2024
SHA: 2f6849b
Reason: push, by JensenPaul

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
spec Relates to the spec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants