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

webgl: Refactor texture validations to take advantage of rust type system #11724

Merged
merged 1 commit into from Jun 25, 2016

Conversation

@emilio
Copy link
Member

emilio commented Jun 11, 2016


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because refactoring.

This commit introduces the WebGLValidator trait, and uses it for multiple
validations in the texture-related WebGL code, to move that logic out of the
already bloated webglrenderingcontext.rs file.

It also creates a type-safe wrapper for some WebGL types, removing all the
unreachable!s there, and introduces a macro for generating them conveniently.

This partially addresses #10693, pending refactor more code to use this
infrastructure, and (possibly?) introducing an AsGLError trait for the errors
to make the error handling happen in WebGLContext.

r? @jdm


This change is Reviewable

@highfive
Copy link

highfive commented Jun 11, 2016

Heads up! This PR modifies the following files:

  • @KiChjang: components/script/dom/mod.rs, components/script/dom/webgl_validations/types.rs, components/script/dom/webgl_validations/mod.rs, components/script/dom/webgltexture.rs, components/script/dom/webglrenderingcontext.rs, components/script/dom/webgl_validations/tex_image_2d.rs
@highfive
Copy link

highfive commented Jun 11, 2016

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@emilio
Copy link
Member Author

emilio commented Jun 11, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 11, 2016

Trying commit 2d97df6 with merge b148b4e...

bors-servo added a commit that referenced this pull request Jun 11, 2016
webgl: Refactor texture validations to take advantage of rust type system

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [x] These changes do not require tests because refactoring.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This commit introduces the `WebGLValidator` trait, and uses it for multiple
validations in the texture-related WebGL code, to move that logic out of the
already bloated `webglrenderingcontext.rs` file.

It also creates a type-safe wrapper for some WebGL types, removing all the
`unreachable!`s there, and introduces a macro for generating them conveniently.

This partially addresses #10693, pending refactor more code to use this
infrastructure, and (possibly?) introducing an `AsGLError` trait for the errors
to make the error handling happen in `WebGLContext`.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11724)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 11, 2016

💔 Test failed - mac-rel-wpt

@highfive
Copy link

highfive commented Jun 11, 2016

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/more/functions/copyTexImage2DBadArgs.html:
  │ FAIL [expected PASS] WebGL test #0: testTexImage2D
  │   → assert_true: testTexImage2D expected true got false
  │ 
  │ reportTestResultsToHarness/&lt;@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:900:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:899:5
  │ runTests@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:188:5
  │ initTests@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:922:5
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:947:7

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/more/functions/texSubImage2D.html:
  └ PASS [expected FAIL] WebGL test #0: testTexSubImage2D

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/textures/texture-npot.html:
  │ FAIL [expected PASS] WebGL test #1: getError expected: INVALID_VALUE. Was NO_ERROR : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE
  │   → assert_true: getError expected: INVALID_VALUE. Was NO_ERROR : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE expected true got false
  │ FAIL [expected PASS] WebGL test #13: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE
  │   → assert_true: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE expected true got false
  │ FAIL [expected PASS] WebGL test #25: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE
  │   → assert_true: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE expected true got false
  │ FAIL [expected PASS] WebGL test #37: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE
  │   → assert_true: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE expected true got false
  │ FAIL [expected PASS] WebGL test #49: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE
  │   → assert_true: getError expected: INVALID_VALUE. Was INVALID_OPERATION : gl.texImage2D with NPOT texture with level &gt; 0 should return INVALID_VALUE expected true got false
  │ 
  │ reportTestResultsToHarness/&lt;@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:87:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:86:7
  │ testFailed@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:152:5
  │ glErrorShouldBe@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1379:5
  │ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:67:3
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:60:1

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/textures/texture-npot.html:
  │ FAIL [expected PASS] WebGL test #35: at (0, 0) expected: 192,192,192,255 was 0,0,0,255
  │   → assert_true: at (0, 0) expected: 192,192,192,255 was 0,0,0,255 expected true got false
  │ </span><span class="stdout">
  │ reportTestResultsToHarness/&lt;@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:87:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:86:7
  │ testFailed@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:152:5
  │ checkCanvasRectColor@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1046:9
  │ checkCanvasRect@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1072:3
  │ checkCanvas@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1095:3
  │ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:125:3
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:60:1

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/textures/texture-npot.html:
  │ FAIL [expected PASS] WebGL test #34: getError expected: NO_ERROR. Was INVALID_OPERATION : gl.texImage2D and gl.generateMipmap with POT texture at level 0 should succeed
  │   → assert_true: getError expected: NO_ERROR. Was INVALID_OPERATION : gl.texImage2D and gl.generateMipmap with POT texture at level 0 should succeed expected true got false
  │ 
  │ reportTestResultsToHarness/&lt;@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:87:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:86:7
  │ testFailed@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:152:5
  │ glErrorShouldBe@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1379:5
  │ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:116:3
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:60:1
@emilio emilio force-pushed the emilio:webgl-refactor branch from 2d97df6 to bbc3008 Jun 12, 2016
@highfive
Copy link

highfive commented Jun 12, 2016

New code was committed to pull request.

@emilio
Copy link
Member Author

emilio commented Jun 12, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2016

Trying commit bbc3008 with merge 169d743...

@emilio emilio force-pushed the emilio:webgl-refactor branch from bbc3008 to 5c85052 Jun 12, 2016
@highfive
Copy link

highfive commented Jun 12, 2016

New code was committed to pull request.

@emilio
Copy link
Member Author

emilio commented Jun 12, 2016

@bors-servo: try

(typo, sorry for the spam)

@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2016

Trying commit 5c85052 with merge 056e2ff...

bors-servo added a commit that referenced this pull request Jun 12, 2016
webgl: Refactor texture validations to take advantage of rust type system

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [x] These changes do not require tests because refactoring.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This commit introduces the `WebGLValidator` trait, and uses it for multiple
validations in the texture-related WebGL code, to move that logic out of the
already bloated `webglrenderingcontext.rs` file.

It also creates a type-safe wrapper for some WebGL types, removing all the
`unreachable!`s there, and introduces a macro for generating them conveniently.

This partially addresses #10693, pending refactor more code to use this
infrastructure, and (possibly?) introducing an `AsGLError` trait for the errors
to make the error handling happen in `WebGLContext`.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11724)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 12, 2016

💔 Test failed - mac-rel-wpt

@highfive
Copy link

highfive commented Jun 12, 2016

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/more/functions/copyTexImage2DBadArgs.html:
  │ FAIL [expected PASS] WebGL test #0: testTexImage2D
  │   → assert_true: testTexImage2D expected true got false
  │ 
  │ reportTestResultsToHarness/&lt;@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:900:7
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:899:5
  │ runTests@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:188:5
  │ initTests@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:922:5
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/more/unit.js:947:7

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/more/functions/texSubImage2D.html:
  └ PASS [expected FAIL] WebGL test #0: testTexSubImage2D

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/textures/texture-npot.html:
  │ FAIL [expected PASS] WebGL test #34: getError expected: NO_ERROR. Was INVALID_OPERATION : gl.texImage2D and gl.generateMipmap with POT texture at level 0 should succeed
  │   → assert_true: getError expected: NO_ERROR. Was INVALID_OPERATION : gl.texImage2D and gl.generateMipmap with POT texture at level 0 should succeed expected true got false
  │ 
  │ reportTestResultsToHarness/&lt;@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:87:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:86:7
  │ testFailed@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:152:5
  │ glErrorShouldBe@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1379:5
  │ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:116:3
  └ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:60:1

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/textures/texture-npot.html:
  │ FAIL [expected PASS] WebGL test #35: at (0, 0) expected: 192,192,192,255 was 0,0,0,255
  │   → assert_true: at (0, 0) expected: 192,192,192,255 was 0,0,0,255 expected true got false
  │ 
  │ reportTestResultsToHarness/&lt;@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:87:9
  │ Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1397:20
  │ test@http://web-platform.test:8000/resources/testharness.js:495:9
  │ reportTestResultsToHarness@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:86:7
  │ testFailed@http://web-platform.test:8000/webgl/conformance-1.0.3/resources/js-test-pre.js:152:5
  │ checkCanvasRectColor@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1046:9
  │ checkCanvasRect@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1072:3
  │ checkCanvas@http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/resources/webgl-test-utils.js:1095:3
  │ @http://web-platform.test:8000/webgl/conformance-1.0.3/conformance/textures/texture-npot.html:125:3
  └ @http://web-platform.test:8000/webgl/conforman</span><span class="stdout">ce-1.0.3/conformance/textures/texture-npot.html:60:1
@emilio emilio force-pushed the emilio:webgl-refactor branch from 5c85052 to f5df6d4 Jun 15, 2016
@highfive
Copy link

highfive commented Jun 15, 2016

New code was committed to pull request.

@cbrewster
Copy link
Member

cbrewster commented Jun 25, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

Testing commit 46c14ac with merge e43e1eb...

bors-servo added a commit that referenced this pull request Jun 25, 2016
webgl: Refactor texture validations to take advantage of rust type system

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [x] These changes do not require tests because refactoring.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This commit introduces the `WebGLValidator` trait, and uses it for multiple
validations in the texture-related WebGL code, to move that logic out of the
already bloated `webglrenderingcontext.rs` file.

It also creates a type-safe wrapper for some WebGL types, removing all the
`unreachable!`s there, and introduces a macro for generating them conveniently.

This partially addresses #10693, pending refactor more code to use this
infrastructure, and (possibly?) introducing an `AsGLError` trait for the errors
to make the error handling happen in `WebGLContext`.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11724)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

💔 Test failed - mac-rel-wpt

@cbrewster
Copy link
Member

cbrewster commented Jun 25, 2016

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

Testing commit 46c14ac with merge 8068076...

bors-servo added a commit that referenced this pull request Jun 25, 2016
webgl: Refactor texture validations to take advantage of rust type system

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors

<!-- Either: -->
- [x] These changes do not require tests because refactoring.

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

This commit introduces the `WebGLValidator` trait, and uses it for multiple
validations in the texture-related WebGL code, to move that logic out of the
already bloated `webglrenderingcontext.rs` file.

It also creates a type-safe wrapper for some WebGL types, removing all the
`unreachable!`s there, and introduces a macro for generating them conveniently.

This partially addresses #10693, pending refactor more code to use this
infrastructure, and (possibly?) introducing an `AsGLError` trait for the errors
to make the error handling happen in `WebGLContext`.

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11724)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

💔 Test failed - android

@cbrewster
Copy link
Member

cbrewster commented Jun 25, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, windows are reusable. Rebuilding only android, linux-rel, mac-rel-wpt...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

💔 Test failed - linux-rel

@cbrewster
Copy link
Member

cbrewster commented Jun 25, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

Previous build results for arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only android, linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

💔 Test failed - linux-rel

@cbrewster
Copy link
Member

cbrewster commented Jun 25, 2016

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

Previous build results for android, arm32, arm64, linux-dev, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows are reusable. Rebuilding only linux-rel...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 25, 2016

@bors-servo bors-servo merged commit 46c14ac into servo:master Jun 25, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@emilio emilio deleted the emilio:webgl-refactor branch Jun 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.