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 upRefactor WebGL implementation to move logic inside the DOM interfaces #6380
Conversation
hoppipolla-critic-bot
commented
Jun 14, 2015
|
Critic review: https://critic.hoppipolla.co.uk/r/5279 This is an external review system which you may optionally use for the code review of your pull request. In order to help critic track your changes, please do not make in-place history rewrites (e.g. via |
|
|
|
DOM changes are blocked until #6150 lands. |
|
|
|
Please rebase on master. |
|
Rebased, also added the |
|
|
|
@bors-servo: r+ Reviewed 9 of 9 files at r4. Comments from the review on Reviewable.io |
all files reviewed at latest revision, all discussions resolved, all commit checks successful.
|
|
Refactor WebGL implementation to move logic inside the DOM interfaces
This improves the encapsulation and consistency in our WebGL
implementation.
Also allows to implement new methods such as `getShaderSource()`.
It will also allow us to use `delete()` in the destructors of them (note
that we will probably want to keep track of them from the context before).
Some concerns:
**Trait method repetition**:
I'm aware that the traits `WebGL{Buffer,Renderbuffer,Framebuffer,Texture}Helpers` are basically the same, but `delete()` and `id()` methods are everywhere. I've thought something like:
```rust
pub trait WebGLIdentifiable {
type WebGLId; // id is sometimes i32 (see WebGLUniformLocation)
fn id(&self) -> Self::WebGLId;
}
pub trait WebGLBindable {
fn bind(&self);
}
pub trait WebGLDeletable {
fn delete(&self);
}
```
But I'd want to know your opinion first.
**`renderer` repetition**:
Thought of moving the field: `renderer: Sender<CanvasMsg>` to `WebGLObject`, but I think it makes it way more complicated to read, and also a bit unnecessary, at least IMO (`WebGLObject` will never interact with the field directly). It would also mean that all `WebGLObject`s should have one, which is true at this moment, but maybe not with WebGL 2, for example.
<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6380)
<!-- Reviewable:end -->
emilio commentedJun 14, 2015
This improves the encapsulation and consistency in our WebGL
implementation.
Also allows to implement new methods such as
getShaderSource().It will also allow us to use
delete()in the destructors of them (notethat we will probably want to keep track of them from the context before).
Some concerns:
Trait method repetition:
I'm aware that the traits
WebGL{Buffer,Renderbuffer,Framebuffer,Texture}Helpersare basically the same, butdelete()andid()methods are everywhere. I've thought something like:But I'd want to know your opinion first.
rendererrepetition:Thought of moving the field:
renderer: Sender<CanvasMsg>toWebGLObject, but I think it makes it way more complicated to read, and also a bit unnecessary, at least IMO (WebGLObjectwill never interact with the field directly). It would also mean that allWebGLObjects should have one, which is true at this moment, but maybe not with WebGL 2, for example.