Skip to content

Stop raising errors during/after device loss #4051

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.

Sign up for GitHub

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

Jump to bottom
Merged
kainino0x merged 2 commits into from
Apr 18, 2023
Merged

Stop raising errors during/after device loss #4051

kainino0x merged 2 commits into from
Apr 18, 2023

Conversation

@kainino0x
Copy link
Contributor

@kainino0x kainino0x commented Apr 18, 2023

Fixes #4017
One of the last two things to complete #1629

@kainino0x kainino0x added the copyediting Pure editorial stuff (copyediting, *.bs file syntax, etc.) label Apr 18, 2023
@kainino0x kainino0x added this to the V1.0 milestone Apr 18, 2023
@kainino0x kainino0x requested a review from toji April 18, 2023 05:23
@kainino0x kainino0x force-pushed the error-after-lost branch 2 times, most recently from 885c32d to 4b4d25e Compare April 18, 2023 05:27
@kainino0x kainino0x mentioned this pull request Apr 18, 2023
Closed
8 tasks
@kainino0x kainino0x added the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Apr 18, 2023
@kainino0x
Stop raising errors during/after device loss
f9568bd 
Fixes 4017
Almost completes 1629
toji
toji approved these changes Apr 18, 2023
View reviewed changes
Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

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

LGTM.

It does make me wonder how we should handle the few situations where we throw exceptions, such as validating texture format features. Does the lost device:

  • Continue to track which features were enabled to deliver consistent results
  • Act as if no features are enabled after the device is lost
  • Stop validating the enums all together (which will still be inconsistent across browsers, because the IDL bindings will reject some of them)

In any case, answering that question doesn't stop this change from going into effect.

@kainino0x
Copy link
Contributor Author

kainino0x commented Apr 18, 2023

I think content-timeline validation should continue to behave normally, your option 1. It would be kind of artificial to do otherwise, as the content timeline can't even change its behavior until it finds out the device is lost (which is after the time the device is actually lost). The other two options have clear issues (2 can cause exceptions to suddenly start popping up in the middle of the program, and 3 has the problem you observed)

@kainino0x kainino0x enabled auto-merge (squash) April 18, 2023 22:21
@kainino0x kainino0x merged commit f59641d into gpuweb:main Apr 18, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Apr 18, 2023

Previews, as seen when this build job started (e2db74b):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@kainino0x kainino0x deleted the error-after-lost branch April 18, 2023 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@toji toji toji approved these changes

Assignees

No one assigned

Labels

copyediting Pure editorial stuff (copyediting, *.bs file syntax, etc.) needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet

Projects

None yet

Milestone

Milestone 0

Development

Successfully merging this pull request may close these issues.

uncapturederror shouldn't fire after device loss

2 participants

@kainino0x @toji