Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd renderbuffer checks; #21485 #21813
Conversation
ghost
commented
Sep 25, 2018
No Taskcluster jobs started for this pull requestThe `allowPullRequests` configuration for this repository (in `.taskcluster.yml` on the
default branch) does not allow starting tasks for this pull request. |
highfive
commented
Sep 25, 2018
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @paulrouget (or someone else) soon. |
highfive
commented
Sep 25, 2018
|
Heads up! This PR modifies the following files:
|
highfive
commented
Sep 25, 2018
|
|
| .limits() | ||
| .max_renderbuffer_size as i32; | ||
|
|
||
| if width > max_size || height > max_size { |
This comment has been minimized.
This comment has been minimized.
jdm
Sep 25, 2018
Member
Why don't we move this check into WebGLRenderingContext::RenderbufferStorage instead? I think it makes more conceptual sense there.
This comment has been minimized.
This comment has been minimized.
sumit0190
Sep 25, 2018
Author
Contributor
Hmm...I thought the WebGLMessage::RenderbufferStorage directly calls the corresponding gl API.
Also, WebGLRenderingContext::RenderbufferStorage already checks for these exact same conditions.
This comment has been minimized.
This comment has been minimized.
jdm
Oct 10, 2018
Member
If the only caller of this method already checks these conditions, then I think this change is redundant.
|
@bors-servo try=wpt |
Add renderbuffer checks; #21485 Added a check for width and height during render buffer creation. I will add a test after the initial review (there are existing tests too, I think). --- <!-- 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 - [X] These changes fix #21485 (github issue number if applicable). <!-- Either: --> - [X] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- 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/21813) <!-- Reviewable:end -->
|
|
|
The tests passed with no unexpected results. |
|
Sorry, I didn't notice that we already check this condition in WebGLRenderingContext::RenderbufferStorage. I don't see a good reason to move the check or duplicate it. |
sumit0190 commentedSep 25, 2018
•
edited by SimonSapin
Added a check for width and height during render buffer creation. I will add a test after the initial review (there are existing tests too, I think).
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is