Issue #8738: bufferSubData and texImage2D argument sanity checks.

[simartin]

Fixes #8738

[frewsxcv]

components/script/dom/webglrenderingcontext.rs, line 931 [r1] (raw file):
could this just be if self.bound_texture_for(target).is_none() { ?

---

Comments from the review on Reviewable.io

[simartin]

components/script/dom/webglrenderingcontext.rs, line 931 [r1] (raw file):
You're right, it's more readable. PR update coming up.

---

Comments from the review on Reviewable.io

[bors-servo]

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

[emilio]

Review status: 0 of 4 files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

---

components/script/dom/webglrenderingcontext.rs, line 931 [r1] (raw file):
Good catch here :)

---

components/script/dom/webglrenderingcontext.rs, line 84 [r2] (raw file):
I think the allocated size should be part of the WebGLBuffer object itself, something similar to what we do with the target field.
What would happen if you re-bind a buffer with data already allocated?

---

components/script/dom/webglrenderingcontext.rs, line 401 [r2] (raw file):
We probably should move almost all this logic to webglbuffer.rs, to a function with a signature like:

pub fn buffer_data(&self, target: u32, data: Vec<u8>, usage: u32) -> WebGLResult<()>;

Where the new size size should be updated.

And leave this function with just the validations and something like:

handle_potential_webgl_error!(self.buffer_data(target, data, usage));

---

components/script/dom/webglrenderingcontext.rs, line 460 [r2] (raw file):
Same as above, this logic should be moved to the WebGLBuffer object.

---

Comments from the review on Reviewable.io

[simartin]

Thanks for the review! I've taken your comments into account and rebased.

---

Comments from the review on Reviewable.io

[emilio]

Looks good to me, modulo those little changes :)

-S-awaiting-review +S-needs-code-changes

---

Review status: 0 of 6 files reviewed at latest revision, 6 unresolved discussions.

---

components/script/dom/webglbuffer.rs, line 21 [r3] (raw file):
I'd prefer this to be called allocated_size or capacity, it looks clearer to me, but it's a matter of taste so if you think it's fine, it can remain this way.

---

components/script/dom/webglbuffer.rs, line 79 [r3] (raw file):
Please send the message to the renderer here instead of doing it in BufferData.

---

components/script/dom/webglrenderingcontext.rs, line 186 [r3] (raw file):
There's a PR to handle this a bit more cleanly (#8970) and avoid repeating us too much, but I'll rebase it if this lands before so no problem :)

---

components/script/dom/webglrenderingcontext.rs, line 492 [r3] (raw file):
This should be InvalidOperation instead of InvalidValue.

---

components/script/dom/webglrenderingcontext.rs, line 501 [r3] (raw file):
Maybe this could use the byte_array_to_slice function?

---

Comments from the review on Reviewable.io

[simartin]

components/script/dom/webglrenderingcontext.rs, line 492 [r3] (raw file):
You're right, thanks!

---

Comments from the review on Reviewable.io

[simartin]

components/script/dom/webglrenderingcontext.rs, line 501 [r3] (raw file):
Did not see this one, thanks!

---

Comments from the review on Reviewable.io

[simartin]

Thanks for the review. Update is coming up.

---

Comments from the review on Reviewable.io

[jdm]

@bors-servo: delegate=ecoal95

[bors-servo]

✌️ @ecoal95 can now approve this pull request

[emilio]

@bors-servo: r+

Nice work, thanks @simartin!

-S-awaiting-review +S-awaiting-merge

---

Reviewed 1 of 2 files at r2, 2 of 5 files at r3, 1 of 1 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit f2fe401 has been approved by ecoal95

[bors-servo]

⌛ Testing commit f2fe401 with merge c1cb940...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt