Implement WebGL GetRenderbufferParameter #20631

fnune · 2018-04-12T17:34:43Z

This needed a bump of gleam to version 0.4.33 for this servo/gleam#162

I think my changes in test expectations are pretty naive and I have to wait for the PR to run on CI to see what the actual impact is. I'd like some guidance on this, too.

./mach build -d does not report any errors
./mach build-geckolib does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement WebGLRenderingContext.getRenderbufferParameter #20514 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2018-04-12T17:34:50Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/webglrenderingcontext.rs, components/script/Cargo.toml, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs
@paulrouget: components/compositing/Cargo.toml, components/servo/Cargo.toml, ports/servo/Cargo.toml
@fitzgen: components/script/dom/webglrenderingcontext.rs, components/script/Cargo.toml, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs
@KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/Cargo.toml, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webgl2renderingcontext.rs

highfive · 2018-04-12T17:34:53Z

Warning

These commits modify unsafe code. Please review it carefully!

fnune · 2018-04-12T17:49:14Z

r?

gootorov · 2018-04-13T17:33:20Z

components/script/dom/webglrenderingcontext.rs

+        self.send_command(WebGLCommand::GetRenderbufferParameter(target, pname, sender));
+
+        match receiver.recv().unwrap() {
+            value if value != 0 => Int32Value(value),


0 can be a valid result, right? I think it should be just Int32Value(receiver.recv().unwrap()).

gootorov · 2018-04-13T17:34:49Z

@bors-servo try

bors-servo · 2018-04-13T17:34:57Z

⌛ Trying commit 6692241 with merge a8818d8...

…parameter, r=<try> Implement WebGL GetRenderbufferParameter This needed a bump of gleam to version 0.4.33 for this servo/gleam#162  I think my changes in test expectations are pretty naive and I have to wait for the PR to run on CI to see what the actual impact is. I'd like some guidance on this, too. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20514 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20631)

gootorov · 2018-04-13T17:41:31Z

components/script/dom/webglrenderingcontext.rs

@@ -2345,6 +2345,41 @@ impl WebGLRenderingContextMethods for WebGLRenderingContext {
        Int32Value(receiver.recv().unwrap())
    }

+    #[allow(unsafe_code)]
+    // https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.7
+    unsafe fn GetRenderbufferParameter(&self, _cx: *mut JSContext, target: u32, pname: u32) -> JSVal {


nit: For functions with many arguments, we usually indent like so

unsafe fn GetRenderbufferParameter( &self, ... ) -> JSVal {

gootorov · 2018-04-13T17:45:03Z

components/script/dom/webglrenderingcontext.rs

+
+        let (sender, receiver) = webgl_channel().unwrap();
+        self.send_command(WebGLCommand::GetRenderbufferParameter(target, pname, sender));
+


The following check is missing:
If the renderbuffer currently bound to target is zero, then INVALID_- OPERATION is generated.
Reference

It's just this, basically :)

if self.bound_renderbuffer.get().is_none() { self.webgl_error(InvalidOperation); return NullValue(); }

bors-servo · 2018-04-13T18:04:40Z

💔 Test failed - linux-rel-wpt

fnune · 2018-04-16T07:25:21Z

I'll get to this in the evening, thanks :) Also I think my expectations for test failures are wrong, I should be updating more .ini files.

fnune · 2018-04-16T16:20:33Z

I pushed changes regarding your suggestions and removed some more "expected to fail" .ini entries. I'm not sure if I interpreted the test failures correctly though? Here they are http://build.servo.org/builders/linux-rel-wpt/builds/8500/steps/shell__4/logs/intermittents.log

gootorov · 2018-04-16T17:03:05Z

Those are known failures, don't worry about them. The relevant ones are in the filtered-wpt-errorsummary.log file.

gootorov · 2018-04-16T17:03:13Z

@bors-servo try

bors-servo · 2018-04-16T17:03:19Z

⌛ Trying commit 6b166ce with merge 6ef4b98...

…parameter, r=<try> Implement WebGL GetRenderbufferParameter This needed a bump of gleam to version 0.4.33 for this servo/gleam#162  I think my changes in test expectations are pretty naive and I have to wait for the PR to run on CI to see what the actual impact is. I'd like some guidance on this, too. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20514 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20631)

bors-servo · 2018-04-16T17:37:28Z

💔 Test failed - linux-rel-css

nox · 2018-04-17T11:41:57Z

r? @nox

bors-servo · 2018-04-20T19:30:07Z

💔 Test failed - linux-rel-wpt

gootorov · 2018-04-20T19:44:11Z

tests/wpt/mozilla/meta/webgl/conformance-1.0.3/conformance/state/gl-object-get-calls.html.ini

@@ -1,29 +1,53 @@
 [gl-object-get-calls.html]
-  expected: ERROR
-  [WebGL test #33: gl.getRenderbufferParameter(gl.RENDERBUFFER, gl.RENDERBUFFER_WIDTH) should be 2. Threw exception TypeError: gl.getRenderbufferParameter is not a function]
+  expected: TIMEOUT


Looking at the last log, I guess this should be OK, then? Not sure why the script generated this to be a TIMEOUT and what's happening with this test in general.

cc @nox to the rescue.

IDK, I'm pushing again without the TIMEOUT

Alright, let's see. Hope it works now :).

gootorov · 2018-04-20T21:52:38Z

@bors-servo try

bors-servo · 2018-04-20T21:52:42Z

⌛ Trying commit ff95682 with merge ed6b30f...

…parameter, r=<try> Implement WebGL GetRenderbufferParameter This needed a bump of gleam to version 0.4.33 for this servo/gleam#162  I think my changes in test expectations are pretty naive and I have to wait for the PR to run on CI to see what the actual impact is. I'd like some guidance on this, too. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20514 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20631)

bors-servo · 2018-04-20T22:55:33Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
State: approved= try=True

fnune · 2018-04-21T08:09:49Z

Finally 😅

gootorov · 2018-04-21T09:33:07Z

Test successful

cc @nox
This can be r+'ed

fnune · 2018-04-24T07:24:06Z

I hope I don't need to solve conflicts again x_D

emilio

Looks good modulo that. Let me know if you prefer to move that in a followup, that's fine for me :)

emilio · 2018-04-24T08:52:55Z

components/script/dom/webglrenderingcontext.rs

+        if self.bound_renderbuffer.get().is_none() {
+            self.webgl_error(InvalidOperation);
+            return NullValue();
+        }


This should probably move after the target and parameter names, to preserve the same order used in https://www.khronos.org/registry/OpenGL-Refpages/es2.0/xhtml/glGetRenderbufferParameteriv.xml.

This needed a bump of gleam to version 0.4.33

Updated by running /mach test-wpt --log-raw /tmp/servo.log ./**/*1.0.3*/conformance/state/gl-object-get-calls.html ...and then ./mach update-wpt /tmp/servo.log

fnune · 2018-04-24T16:18:32Z

r?

@emilio I moved the check to the place suggested. I thought of putting it there initially, but I thought it might be cheaper to check at the start because the second check needs target_matches and pname_matches to be assigned in order to work. I guess it's not important anyway.

emilio · 2018-04-25T08:18:34Z

@bors-servo r+

Yeah, in general optimizing for error paths isn't really necessary. And in this case it's observable, so there's not much we can do about it anyway.

bors-servo · 2018-04-25T08:18:35Z

📌 Commit a30674a has been approved by emilio

bors-servo · 2018-04-25T08:18:41Z

⌛ Testing commit a30674a with merge f1a06e0...

…parameter, r=emilio Implement WebGL GetRenderbufferParameter This needed a bump of gleam to version 0.4.33 for this servo/gleam#162  I think my changes in test expectations are pretty naive and I have to wait for the PR to run on CI to see what the actual impact is. I'd like some guidance on this, too. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach build-geckolib` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20514 (github issue number if applicable).  - [x] There are tests for these changes OR - [ ] These changes do not require tests because _____    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20631)

bors-servo · 2018-04-25T09:10:48Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing f1a06e0 to master...

highfive assigned pcwalton Apr 12, 2018

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

gootorov reviewed Apr 13, 2018

View reviewed changes

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

nox removed the S-awaiting-review There is new code that needs to be reviewed. label Apr 16, 2018

fnune force-pushed the add-webgl-get-renderbuffer-parameter branch from 6692241 to 3cefd52 Compare April 16, 2018 16:18

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 16, 2018

fnune force-pushed the add-webgl-get-renderbuffer-parameter branch from 3cefd52 to a6b6568 Compare April 16, 2018 16:19

fnune force-pushed the add-webgl-get-renderbuffer-parameter branch from a6b6568 to 6b166ce Compare April 16, 2018 16:28

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

highfive assigned nox and unassigned pcwalton Apr 17, 2018

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

gootorov reviewed Apr 20, 2018

View reviewed changes

fnune force-pushed the add-webgl-get-renderbuffer-parameter branch from 4b4c286 to ff95682 Compare April 20, 2018 21:44

highfive removed the S-tests-failed The changes caused existing tests to fail. label Apr 20, 2018

emilio approved these changes Apr 24, 2018

View reviewed changes

fnune added 2 commits April 24, 2018 18:16

Implement WebGL GetRenderbufferParameter

58760d9

This needed a bump of gleam to version 0.4.33

Update test failure expectations for gl.getUniform

a30674a

Updated by running /mach test-wpt --log-raw /tmp/servo.log ./**/*1.0.3*/conformance/state/gl-object-get-calls.html ...and then ./mach update-wpt /tmp/servo.log

fnune force-pushed the add-webgl-get-renderbuffer-parameter branch from ff95682 to a30674a Compare April 24, 2018 16:17

highfive assigned emilio and unassigned nox Apr 25, 2018

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. S-needs-rebase There are merge conflict errors. labels Apr 25, 2018

bors-servo merged commit a30674a into servo:master Apr 25, 2018

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

fnune deleted the add-webgl-get-renderbuffer-parameter branch April 25, 2018 09:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement WebGL GetRenderbufferParameter #20631

Implement WebGL GetRenderbufferParameter #20631

fnune commented Apr 12, 2018 •

edited

Loading

highfive commented Apr 12, 2018

highfive commented Apr 12, 2018

fnune commented Apr 12, 2018

gootorov Apr 13, 2018 •

edited

Loading

gootorov commented Apr 13, 2018

bors-servo commented Apr 13, 2018

gootorov Apr 13, 2018 •

edited

Loading

gootorov Apr 13, 2018 •

edited

Loading

gootorov Apr 13, 2018

bors-servo commented Apr 13, 2018

fnune commented Apr 16, 2018

fnune commented Apr 16, 2018

gootorov commented Apr 16, 2018

gootorov commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018

nox commented Apr 17, 2018

bors-servo commented Apr 20, 2018

gootorov Apr 20, 2018

fnune Apr 20, 2018

gootorov Apr 20, 2018

gootorov commented Apr 20, 2018

bors-servo commented Apr 20, 2018

bors-servo commented Apr 20, 2018

fnune commented Apr 21, 2018

gootorov commented Apr 21, 2018

fnune commented Apr 24, 2018

emilio left a comment

emilio Apr 24, 2018

fnune commented Apr 24, 2018

emilio commented Apr 25, 2018

bors-servo commented Apr 25, 2018

bors-servo commented Apr 25, 2018

bors-servo commented Apr 25, 2018


		let (sender, receiver) = webgl_channel().unwrap();
		self.send_command(WebGLCommand::GetRenderbufferParameter(target, pname, sender));

Implement WebGL GetRenderbufferParameter #20631

Implement WebGL GetRenderbufferParameter #20631

Conversation

fnune commented Apr 12, 2018 • edited Loading

highfive commented Apr 12, 2018

highfive commented Apr 12, 2018

fnune commented Apr 12, 2018

gootorov Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

gootorov commented Apr 13, 2018

bors-servo commented Apr 13, 2018

gootorov Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

gootorov Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

gootorov Apr 13, 2018

Choose a reason for hiding this comment

bors-servo commented Apr 13, 2018

fnune commented Apr 16, 2018

fnune commented Apr 16, 2018

gootorov commented Apr 16, 2018

gootorov commented Apr 16, 2018

bors-servo commented Apr 16, 2018

bors-servo commented Apr 16, 2018

nox commented Apr 17, 2018

bors-servo commented Apr 20, 2018

gootorov Apr 20, 2018

Choose a reason for hiding this comment

fnune Apr 20, 2018

Choose a reason for hiding this comment

gootorov Apr 20, 2018

Choose a reason for hiding this comment

gootorov commented Apr 20, 2018

bors-servo commented Apr 20, 2018

bors-servo commented Apr 20, 2018

fnune commented Apr 21, 2018

gootorov commented Apr 21, 2018

fnune commented Apr 24, 2018

emilio left a comment

Choose a reason for hiding this comment

emilio Apr 24, 2018

Choose a reason for hiding this comment

fnune commented Apr 24, 2018

emilio commented Apr 25, 2018

bors-servo commented Apr 25, 2018

bors-servo commented Apr 25, 2018

bors-servo commented Apr 25, 2018

fnune commented Apr 12, 2018 •

edited

Loading

gootorov Apr 13, 2018 •

edited

Loading

gootorov Apr 13, 2018 •

edited

Loading

gootorov Apr 13, 2018 •

edited

Loading