webgl: implement bufferData #14847

Merged
merged 4 commits into from Jan 5, 2017

Projects

None yet

7 participants

@anholt
Contributor
anholt commented Jan 4, 2017 edited

Adds support for the other overload of bufferData, fixing many conformance tests. In the process I had to fix the webidl codegen in the overload-distinguished-by-an-object case. Also includes a little fix for glEnable() validation.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #__ (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests because _____

This change is Reviewable

anholt added some commits Jan 2, 2017
@anholt anholt webgl: Allow enable/disable of STENCIL_TEST c6f73a6
@anholt anholt Fix is_null_or_undefined() call in codegen to be snake_case.
After a bit of digging, I couldn't find when it was camelCase.  This
started getting generated when I added an overload in webgl.
d90499a
@mbrubeck mbrubeck was assigned by highfive Jan 4, 2017
@highfive
highfive commented Jan 4, 2017

Heads up! This PR modifies the following files:

  • @kichjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl
  • @fitzgen: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl
  • @emilio: components/script/dom/webglrenderingcontext.rs
@@ -398,7 +398,7 @@ def pickFirstSignature(condition, filterLambda):
return False
# First check for null or undefined
- pickFirstSignature("%s.isNullOrUndefined()" % distinguishingArg,
+ pickFirstSignature("%s.get().is_null_or_undefined()" % distinguishingArg,
@jdm
jdm Jan 4, 2017 Member

You found a part of codegen that was imported from gecko and has never been used in servo yet. Congratulations!

@jdm

Nice work!

+ pickFirstSignature("%s.get().is_object() && "
+ "{ rooted!(in(cx) let obj = %s.get().to_object()); "
+ "let mut is_date = false; "
+ "JS_ObjectIsDate(cx, obj.handle(), &mut is_date); "
@jdm
jdm Jan 4, 2017 Member

Let's assert that JS_ObjectIsDate returns true in order to catch any failures in the JS engine.

@jdm jdm assigned jdm and unassigned mbrubeck Jan 4, 2017
@emilio
emilio approved these changes Jan 4, 2017 View changes

Nice! The WebGL parts look great to me.

// FIXME(dmarcos) The function below is the original function in the webIdl:
// void bufferData(GLenum target, BufferDataSource? data, GLenum usage);
// The Code generator doesn't handle BufferDataSource so we're using 'object?'
// in the meantime, and marking the function as [Throws], so we can handle
// the type error from inside.
[Throws]
void bufferData(GLenum target, object? data, GLenum usage);
+ [Throws]
+ void bufferData(GLenum target, GLsizeiptr size, GLenum usage);
@emilio
emilio Jan 4, 2017 Member

This needs to be [Throws] just because the overload is [Throws] too, right?

If so, mind adding a FIXME here so when we have proper ArrayBuffer WebIDL support we remove both?

@nox
nox Jan 5, 2017 Member

Could you also file a bug about the [Throws] being unneeded in this case? Did codegen fail when you tried without it?

+ _ => return Ok(self.webgl_error(InvalidEnum)),
+ }
+
+ let data = vec![0u8; size as usize];
@emilio
emilio Jan 4, 2017 Member

This is slightly annoying in the sense that we can allocate an unbound buffer when called by the user, but I guess there's no way to allocate this in a fallible way (and there's no need to block on that I guess).

Adding a comment would be helpful if you want :)

@emilio
Member
emilio commented Jan 4, 2017

(Of course, @jdm's comment needs fixing also)

anholt added some commits Jan 4, 2017
@anholt anholt Fix JS_ObjectIsDate() call in webidl codegen.
We were missing the import, and the prototype of the function has
since changed.  Partial fix for #10675
af380c2
@anholt anholt webgl: Add missing overload of bufferData().
I was writing this to fix the error in gl-enum-tests.html test (it now
gets farther along before it fails due to us missing
getTexParameter()), but it turned out that a lot of the conformance
tests were failing due to it.
4c656f1
@anholt
Contributor
anholt commented Jan 5, 2017

Looks like Travis ran out of space.

@emilio
Member
emilio commented Jan 5, 2017

@bors-servo r+

Thanks Eric :)

Travis failure is known unfortunately.

@bors-servo
Contributor

📌 Commit 4c656f1 has been approved by emilio

@highfive highfive assigned emilio and unassigned jdm Jan 5, 2017
@bors-servo
Contributor

⌛️ Testing commit 4c656f1 with merge 4216c16...

@bors-servo bors-servo added a commit that referenced this pull request Jan 5, 2017
@bors-servo bors-servo Auto merge of #14847 - anholt:webgl-bufferdata, r=emilio
webgl: implement bufferData

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

Adds support for the other overload of bufferData, fixing many conformance tests.  In the process I had to fix the webidl codegen in the overload-distinguished-by-an-object case.  Also includes a little fix for glEnable() validation.

---
<!-- 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
- [ ] These changes fix #__ (github issue number if applicable).

<!-- Either: -->
- [x] There are tests for these changes OR
- [ ] These changes do not require tests because _____

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14847)
<!-- Reviewable:end -->
4216c16
@bors-servo bors-servo merged commit 4c656f1 into servo:master Jan 5, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment