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.

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

Change GPUError to a base class instead of a union #2853

Merged
merged 2 commits into from
May 16, 2022
Merged

Change GPUError to a base class instead of a union #2853

merged 2 commits into from
May 16, 2022

Conversation

kainino0x
Copy link
Contributor

@kainino0x kainino0x commented May 9, 2022 •
edited by pr-preview bot
Loading

Closes #2750
Related to #1884

Preview | Diff

@kainino0x kainino0x added the tacit resolution queue Editors have agreed and intend to land if no feedback is given label May 9, 2022
@kainino0x kainino0x requested a review from toji May 9, 2022 23:08
@kainino0x kainino0x added this to the V1.0 milestone May 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented May 9, 2022

Previews, as seen when this build job started (72100f4):
WebGPU | IDL
WGSL
Explainer

toji
toji reviewed May 9, 2022
View reviewed changes
spec/index.bs Outdated Show resolved Hide resolved
spec/index.bs Outdated Show resolved Hide resolved
@kainino0x @toji
Apply suggestions from code review
19ddaeb 
Co-authored-by: Brandon Jones <tojiro@gmail.com>
toji
toji approved these changes May 11, 2022
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!

@kainino0x kainino0x removed the tacit resolution queue Editors have agreed and intend to land if no feedback is given label May 16, 2022
@kainino0x kainino0x merged commit a7f36ce into gpuweb:main May 16, 2022
@kainino0x kainino0x deleted the error-inheritance branch May 16, 2022 22:08
github-actions bot added a commit that referenced this pull request May 16, 2022
@kainino0x @github-actions
Change GPUError to a base class instead of a union (#2853)
2514cad 
SHA: a7f36ce
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
github-actions bot added a commit that referenced this pull request May 16, 2022
@kainino0x @github-actions
Change GPUError to a base class instead of a union (#2853)
5c26692 
SHA: a7f36ce
Reason: push, by @kainino0x

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
aarongable pushed a commit to chromium/chromium that referenced this pull request May 18, 2022
@toji
Update GPUError interface hierarchy
e42a232 
The spec updated in gpuweb/gpuweb#2853 to change
the error types from a union to extended interfaces from a GPUError base
class. IDL slightly differs from current spec text to fix an issue with
the way the constructors were declared.

Bug: 1326472
Change-Id: I83d1d840ef4b57b25980cb8d0da35dea3a0ead82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3649688
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004905}
aarongable pushed a commit to chromium/chromium that referenced this pull request May 18, 2022
@scott-little
Revert "Update GPUError interface hierarchy"
88c2536 
This reverts commit e42a232.

Reason for revert: Looks like this broke compile: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-archive-rel/67275/overview

Original change's description:
> Update GPUError interface hierarchy
>
> The spec updated in gpuweb/gpuweb#2853 to change
> the error types from a union to extended interfaces from a GPUError base
> class. IDL slightly differs from current spec text to fix an issue with
> the way the constructors were declared.
>
> Bug: 1326472
> Change-Id: I83d1d840ef4b57b25980cb8d0da35dea3a0ead82
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3649688
> Reviewed-by: Corentin Wallez <cwallez@chromium.org>
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> Commit-Queue: Brandon Jones <bajones@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1004905}

Bug: 1326472
Change-Id: Ib2f2c427c99fcad09f4f9d201cc224fa7a148486
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654479
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Scott Little <sclittle@chromium.org>
Owners-Override: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004923}
aarongable pushed a commit to chromium/chromium that referenced this pull request May 18, 2022
@toji
Update GPUError interface hierarchy (Take 2)
c1e6757 
The spec updated in gpuweb/gpuweb#2853 to change
the error types from a union to extended interfaces from a GPUError base
class. IDL slightly differs from current spec text to fix an issue with
the way the constructors were declared.

This CL removes the old union include and gn entries, which caused a
compilation failure on the tree with the last attempt.

Bug: 1326472
Change-Id: I4001096f698eb75eea7cbd570a4640ec1182562b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654655
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1005019}
@kainino0x kainino0x mentioned this pull request Jun 13, 2022
Closed
@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 Jun 29, 2022
jdarpinian pushed a commit to jdarpinian/gpuweb that referenced this pull request Aug 12, 2022
@kainino0x @toji @jdarpinian
Change GPUError to a base class instead of a union (gpuweb#2853)
a550052 
* Change GPUError to a base class instead of a union

* Apply suggestions from code review

Co-authored-by: Brandon Jones <tojiro@gmail.com>

Co-authored-by: Brandon Jones <tojiro@gmail.com>
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 17, 2022
@juj
IDL: Update to match changes to GPU error type structure. gpuweb/gpuw…
361a014 
…eb#2853

WebGPU spec still not great, does not have a general error code. See also gpuweb/gpuweb#2750 and gpuweb/gpuweb#1884 .
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@toji @copybara-github
Update GPUError interface hierarchy
f16cb33 
The spec updated in gpuweb/gpuweb#2853 to change
the error types from a union to extended interfaces from a GPUError base
class. IDL slightly differs from current spec text to fix an issue with
the way the constructors were declared.

Bug: 1326472
Change-Id: I83d1d840ef4b57b25980cb8d0da35dea3a0ead82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3649688
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004905}
NOKEYCHECK=True
GitOrigin-RevId: e42a232659787b7037735718d32b3253ef2038d1
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@scott-little @copybara-github
Revert "Update GPUError interface hierarchy"
d5431f4 
This reverts commit e42a232659787b7037735718d32b3253ef2038d1.

Reason for revert: Looks like this broke compile: https://ci.chromium.org/ui/p/chromium/builders/ci/linux-archive-rel/67275/overview

Original change's description:
> Update GPUError interface hierarchy
>
> The spec updated in gpuweb/gpuweb#2853 to change
> the error types from a union to extended interfaces from a GPUError base
> class. IDL slightly differs from current spec text to fix an issue with
> the way the constructors were declared.
>
> Bug: 1326472
> Change-Id: I83d1d840ef4b57b25980cb8d0da35dea3a0ead82
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3649688
> Reviewed-by: Corentin Wallez <cwallez@chromium.org>
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>
> Commit-Queue: Brandon Jones <bajones@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#1004905}

Bug: 1326472
Change-Id: Ib2f2c427c99fcad09f4f9d201cc224fa7a148486
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654479
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Scott Little <sclittle@chromium.org>
Owners-Override: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1004923}
NOKEYCHECK=True
GitOrigin-RevId: 88c2536b797553a94da4f975fc5af4d6511f54a6
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@toji @copybara-github
Update GPUError interface hierarchy (Take 2)
d2938f8 
The spec updated in gpuweb/gpuweb#2853 to change
the error types from a union to extended interfaces from a GPUError base
class. IDL slightly differs from current spec text to fix an issue with
the way the constructors were declared.

This CL removes the old union include and gn entries, which caused a
compilation failure on the tree with the last attempt.

Bug: 1326472
Change-Id: I4001096f698eb75eea7cbd570a4640ec1182562b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3654655
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Brandon Jones <bajones@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1005019}
NOKEYCHECK=True
GitOrigin-RevId: c1e67575a6f8415e3861a6eed96242b1531c6399
@lokokung
Copy link
Contributor

lokokung commented Dec 6, 2022

Updated CTS draft issue val: error scope, so removing 'needs-cts-label'.

@lokokung lokokung removed the needs-cts-issue This change requires tests (or would need tests if accepted), but may not have a CTS issue filed yet label Dec 6, 2022
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

@kdashg kdashg kdashg approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Milestone 0
Development

Successfully merging this pull request may close these issues.
4 participants
@kainino0x @lokokung @toji @kdashg