Implement stencil fns for WebGLRenderingContext #10670

KiChjang · 2016-04-17T19:58:05Z

Depends on servo/webrender#261.

Closes #10659.

This change is

highfive · 2016-04-17T19:58:09Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl
@emilio: components/script/dom/webglrenderingcontext.rs

highfive · 2016-04-17T19:58:10Z

Warning

These commits modify script code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-04-17T19:59:02Z

components/script/dom/webglrenderingcontext.rs

+
+    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.3
+    fn StencilOpSeparate(&self, face: u32, fail: u32, zfail: u32, zpass: u32) {
+        self.ipc_renderer


I'm not sure if these methods need to validate their parameters as described here.

Yes, all parameters should be validated first taking in account the GLES2.0 spec, and later taking in account any details that the WebGL spec could specify.

jdm · 2016-04-17T20:00:39Z

r? @emilio

cbrewster · 2016-04-17T20:03:26Z

components/script/dom/webglrenderingcontext.rs

@@ -1084,6 +1084,48 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
            .unwrap()
    }

+    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.3
+    fn StencilFunc(&self, func: u32, ref_: i32, mask: u32) {


I think these should validate the func parameter as described here

In which case, should these functions have a [Throws] WebIDL attribute added to them?

Nope! WebGL errors are tracked via gl.getError(), not via return values or exceptions.

I do not think so, none of the other functions that can result in a WebGL error seem to have a [Throws] WebIDL attribute. Although I am sure @emilio would have a better answer.

KiChjang · 2016-04-18T01:49:24Z

Added checks for enum values with the stencil functions.

dzbarsky · 2016-04-18T05:45:26Z

components/script/dom/webglrenderingcontext.rs

+            _ => return self.webgl_error(InvalidEnum),
+        }
+
+        match fail {


it might be nice if these were factored out

KiChjang · 2016-04-18T14:31:28Z

Created a new function for checking stencil op parameters.

bors-servo · 2016-04-18T18:47:04Z

☔ The latest upstream changes (presumably #10667) made this pull request unmergeable. Please resolve the merge conflicts.

KiChjang · 2016-04-18T20:32:45Z

Is now blocked on #10678.

KiChjang · 2016-04-19T16:59:02Z

Should be ok to review now.

emilio · 2016-04-19T17:24:53Z

Looks good to me, I usually prefer the early return approach instead of if else, but feel free to r=me if you think it's not worth the change.

@bors-servo: try

Reviewed 1 of 5 files at r1, 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

Comments from Reviewable

bors-servo · 2016-04-19T17:24:56Z

⌛ Trying commit 1107c23 with merge 1d0fc27...

Implement stencil fns for WebGLRenderingContext Depends on servo/webrender#261. Closes #10659.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10670)

bors-servo · 2016-04-19T18:43:24Z

💔 Test failed - mac-rel-wpt

jdm · 2016-04-22T01:57:25Z

It seems likely that #10790 is not actually intermittent and is a permanent failure exposed by the current set of changes.

jdm · 2016-04-22T01:58:01Z

Yeah, it's consistently failed on OS X every time there hasn't been a linux failure first.

emilio · 2016-04-22T09:52:42Z

That test should be fixed by #10443.

Look at this comment though, to see why new correct implementations can lead to new test failures.

bors-servo · 2016-04-22T19:12:21Z

☔ The latest upstream changes (presumably #10443) made this pull request unmergeable. Please resolve the merge conflicts.

KiChjang · 2016-04-24T06:41:30Z

@bors-servo try

bors-servo · 2016-04-24T06:41:33Z

⌛ Trying commit 2946fbe with merge fc1eab7...

Implement stencil fns for WebGLRenderingContext Depends on servo/webrender#261. Closes #10659.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10670)

bors-servo · 2016-04-24T07:53:39Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

KiChjang · 2016-04-24T19:40:57Z

@bors-servo r=emilio

bors-servo · 2016-04-24T19:40:58Z

📌 Commit 2946fbe has been approved by emilio

bors-servo · 2016-04-24T19:41:00Z

⌛ Testing commit 2946fbe with merge 7804173...

Implement stencil fns for WebGLRenderingContext Depends on servo/webrender#261. Closes #10659.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10670)

bors-servo · 2016-04-24T20:53:58Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

emilio · 2016-04-24T21:28:41Z

I'd rather have waited for #10806 to land... But well, it landed so... :P

highfive assigned glennw Apr 17, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 17, 2016

KiChjang reviewed Apr 17, 2016
View reviewed changes

highfive assigned emilio and unassigned glennw Apr 17, 2016

cbrewster reviewed Apr 17, 2016
View reviewed changes

KiChjang force-pushed the webgl-stencils branch 3 times, most recently from 0288fde to 8f92097 Compare April 18, 2016 01:48

dzbarsky reviewed Apr 18, 2016
View reviewed changes

KiChjang force-pushed the webgl-stencils branch from 8f92097 to 2da487e Compare April 18, 2016 14:25

KiChjang force-pushed the webgl-stencils branch from 2da487e to 4a16619 Compare April 18, 2016 15:32

highfive added the S-needs-rebase There are merge conflict errors. label Apr 18, 2016

KiChjang force-pushed the webgl-stencils branch 2 times, most recently from 6d767db to 23eac18 Compare April 18, 2016 19:29

KiChjang removed the S-needs-rebase There are merge conflict errors. label Apr 18, 2016

KiChjang force-pushed the webgl-stencils branch 2 times, most recently from 1fe62f0 to 1107c23 Compare April 19, 2016 16:58

highfive added the S-tests-failed The changes caused existing tests to fail. label Apr 22, 2016

highfive added the S-needs-rebase There are merge conflict errors. label Apr 22, 2016

Implement stencil fns for WebGLRenderingContext

2946fbe

KiChjang force-pushed the webgl-stencils branch from 1107c23 to 2946fbe Compare April 24, 2016 06:41

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Apr 24, 2016

KiChjang removed the S-needs-rebase There are merge conflict errors. label Apr 24, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Apr 24, 2016

bors-servo merged commit 2946fbe into servo:master Apr 24, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Apr 24, 2016

This was referenced Apr 24, 2016

Adding default config directories. #10498

Merged

Extract out 'net/storage_thread' into separate components. #10768

Closed

Investigations for #10760 #10798

Closed

emilio mentioned this pull request Apr 24, 2016

Enable webgl tests. #10806

Merged

KiChjang deleted the webgl-stencils branch July 24, 2016 10:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement stencil fns for WebGLRenderingContext #10670

Implement stencil fns for WebGLRenderingContext #10670

KiChjang commented Apr 17, 2016

highfive commented Apr 17, 2016

highfive commented Apr 17, 2016

KiChjang Apr 17, 2016

emilio Apr 17, 2016

jdm commented Apr 17, 2016

cbrewster Apr 17, 2016

KiChjang Apr 17, 2016 •

edited

Loading

emilio Apr 17, 2016

cbrewster Apr 17, 2016 •

edited

Loading

KiChjang commented Apr 18, 2016

dzbarsky Apr 18, 2016

KiChjang commented Apr 18, 2016

bors-servo commented Apr 18, 2016

KiChjang commented Apr 18, 2016

KiChjang commented Apr 19, 2016

emilio commented Apr 19, 2016

bors-servo commented Apr 19, 2016

bors-servo commented Apr 19, 2016

jdm commented Apr 22, 2016

jdm commented Apr 22, 2016

emilio commented Apr 22, 2016

bors-servo commented Apr 22, 2016

KiChjang commented Apr 24, 2016

bors-servo commented Apr 24, 2016

bors-servo commented Apr 24, 2016

KiChjang commented Apr 24, 2016

bors-servo commented Apr 24, 2016

bors-servo commented Apr 24, 2016

bors-servo commented Apr 24, 2016

emilio commented Apr 24, 2016

Implement stencil fns for WebGLRenderingContext #10670

Implement stencil fns for WebGLRenderingContext #10670

Conversation

KiChjang commented Apr 17, 2016

highfive commented Apr 17, 2016

highfive commented Apr 17, 2016

KiChjang Apr 17, 2016

Choose a reason for hiding this comment

emilio Apr 17, 2016

Choose a reason for hiding this comment

jdm commented Apr 17, 2016

cbrewster Apr 17, 2016

Choose a reason for hiding this comment

KiChjang Apr 17, 2016 • edited Loading

Choose a reason for hiding this comment

emilio Apr 17, 2016

Choose a reason for hiding this comment

cbrewster Apr 17, 2016 • edited Loading

Choose a reason for hiding this comment

KiChjang commented Apr 18, 2016

dzbarsky Apr 18, 2016

Choose a reason for hiding this comment

KiChjang commented Apr 18, 2016

bors-servo commented Apr 18, 2016

KiChjang commented Apr 18, 2016

KiChjang commented Apr 19, 2016

emilio commented Apr 19, 2016

bors-servo commented Apr 19, 2016

bors-servo commented Apr 19, 2016

jdm commented Apr 22, 2016

jdm commented Apr 22, 2016

emilio commented Apr 22, 2016

bors-servo commented Apr 22, 2016

KiChjang commented Apr 24, 2016

bors-servo commented Apr 24, 2016

bors-servo commented Apr 24, 2016

KiChjang commented Apr 24, 2016

bors-servo commented Apr 24, 2016

bors-servo commented Apr 24, 2016

bors-servo commented Apr 24, 2016

emilio commented Apr 24, 2016

KiChjang Apr 17, 2016 •

edited

Loading

cbrewster Apr 17, 2016 •

edited

Loading