Issue #8462: Add support for BufferSubData, CompressedTexImage2D and CompressedSubTexImage2D and reenable individual webgl WPT tests

[simartin]

Fixes #8462

[highfive]

Warning

• These commits modify unsafe code. Please review it carefully!

[jdm]

@ecoal95 Are you interested in taking a first pass at this review?

[emilio]

Yeah, sure!

I'll take a look at it as soon as I arrive home today :P

El sáb, 28 de noviembre de 2015 03:56 p.m., Josh Matthews <
notifications@github.com> escribió:

@ecoal95 https://github.com/ecoal95 Are you interested in taking a
first pass at this review?

—
Reply to this email directly or view it on GitHub
#8712 (comment).

[emilio]

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

---

components/script/dom/webglrenderingcontext.rs, line 406 [r1] (raw file):
Per the WebGL spec (5.14.5):

If data is null then an INVALID_VALUE error is generated.

Returning self.webgl_error(InvalidValue) here will be enough.

---

components/script/dom/webglrenderingcontext.rs, line 407 [r1] (raw file):
May you add a basic validation of the target parameter here? Something similar to what we have here.

If you add it to BufferData too it will be great too :)

We should also validate that offset is not negative (GLES 2.0 spec):

GL_INVALID_VALUE is generated if offset or size is negative, or if together they define a region of memory that extends beyond the buffer object's allocated data store.

The same paragraph says we should also validate that the offset and the size of the ArrayBufferView combined don't define a region inside the allocated one (via BufferData).

Not doing it this way will generate errors in the paint task instead of here (which is unexpected), but if @jdm agrees, it can be left as a followup (since it doesn't involve unsafety, just weird error behaviour, and is more difficult to implement). In that case, a FIXME comment here with the issue number will be appropriate I think, and if you CC me to keep track of it, or implement it when I have the time, it will be great :)

---

components/script/dom/webglrenderingcontext.rs, line 413 [r1] (raw file):
Not sure if a panic here is adequate enough... Maybe we could just generate and InvalidValue error, and leave a comment with a link to the relevant issue about code generation. This applies to BufferData too.

I'd want to know what @jdm thinks about it though.

---

components/script/dom/webglrenderingcontext.rs, line 435 [r1] (raw file):
I think right now we should always generate an InvalidEnum error, since we don't have any extensions which add compressed texture image support. It's not about the number of extensions, but about if we support any extension which add compressed texture image support (which we don't).

So I'll also remove the CompressedTexImage2D message and the code in the paint task, and leave a return self.webgl_error(InvalidEnum) here.

---

components/script/dom/webglrenderingcontext.rs, line 472 [r1] (raw file):
Same remark here, I'll be fine leaving this functions as one-liners with a comment saying something like:

Right now we don't support any compressed image formats. This is spec-complaint (see: // https://www.khronos.org/registry/webgl/specs/latest/1.0/#COMPRESSED_TEXTURE_SUPPORT)

---

Comments from the review on Reviewable.io

[emilio]

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

---

components/script/dom/webglrenderingcontext.rs, line 407 [r1] (raw file):
Sorry, s/don't define a region/define a region here.

---

Comments from the review on Reviewable.io

[simartin]

Thanks a lot for the review. Sending an updated diff shortly.

---

Review status: 0 of 7 files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[emilio]

Nice work! Just some minor fixes and a little clean-up pending and I think this will be ready to merge! :)

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

---

Reviewed 7 of 7 files at r1.
Review status: 6 of 7 files reviewed at latest revision, 9 unresolved discussions.

---

components/canvas_traits/lib.rs, line 146 [r2] (raw file):
Please remove this messages and associated code in webgl_paint_task.rs for now, until it's implemented.

---

components/script/dom/webglrenderingcontext.rs, line 384 [r2] (raw file):
This should be an InvalidEnum error.

Also may you add this just below this match? (just realized this validation was also missing from the initial implementation)

match usage {
    constants::STREAM_DRAW |
    constants::STATIC_DRAW |
    constants::DYNAMIC_DRAW => (),
    _ => return sef.webgl_error(InvalidEnum),
}

---

components/script/dom/webglrenderingcontext.rs, line 388 [r2] (raw file):
Please generate an InvalidValue error here if you can (sorry for the extra work :/).

From the WebGL spec (5.14.5):

If the passed data is null then an INVALID_VALUE error is generated.

---

components/script/dom/webglrenderingcontext.rs, line 395 [r2] (raw file):
May you add a comment here pointing to #5014? :)

---

components/script/dom/webglrenderingcontext.rs, line 412 [r2] (raw file):
This should be an InvalidEnum error, just as above.

---

components/script/dom/webglrenderingcontext.rs, line 426 [r2] (raw file):
Same issue number than in BufferData.

---

components/script/dom/webglrenderingcontext.rs, line 432 [r2] (raw file):
May you add the issue number for #8738?

---

components/script/dom/webglrenderingcontext.rs, line 445 [r2] (raw file):
Since this method always returns the same error (InvalidEnum), regardless of the value of target, I'd remove the match here for now, until an extension is implemented.

---

components/script/dom/webglrenderingcontext.rs, line 460 [r2] (raw file):
ditto.

---

Comments from the review on Reviewable.io

[simartin]

components/script/dom/webglrenderingcontext.rs, line 384 [r2] (raw file):
I put InvalidOperation because that's how I interpreted the following in https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.14.5: "A given WebGLBuffer object may only be bound to one of the ARRAY_BUFFER or ELEMENT_ARRAY_BUFFER target in its lifetime. An attempt to bind a buffer object to the other target will generate an INVALID_OPERATION error". Am I misunderstanding the sentence and its consequence on the other functions from that paragraph?

---

Comments from the review on Reviewable.io

[simartin]

components/script/dom/webglrenderingcontext.rs, line 412 [r2] (raw file):
Same as above

---

Comments from the review on Reviewable.io

[emilio]

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

---

Review status: 4 of 7 files reviewed at latest revision, 2 unresolved discussions.

---

components/script/dom/webglrenderingcontext.rs, line 384 [r2] (raw file):
That means that when the buffer is bound to ARRAY_BUFFER, then it can't be re-bound to ELEMENT_ARRAY_BUFFER. That logic is already handled here: https://github.com/servo/servo/blob/master/components/script/dom/webglbuffer.rs#L59

In this case it's an InvalidEnum error.

---

components/script/dom/webglrenderingcontext.rs, line 412 [r2] (raw file):
Same here :P

---

Comments from the review on Reviewable.io

[jdm]

I agree with @ecoal95 on both points he brought up that referenced me.

[jdm]

@bors-servo: delegate=ecoal95

[bors-servo]

✌️ @ecoal95 can now approve this pull request

[emilio]

Great work! Thanks @simartin :)

@bors-servo: r+

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

---

Reviewed 1 of 1 files at r2, 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit f79e152 has been approved by ecoal95

[bors-servo]

⌛ Testing commit f79e152 with merge 5ee6fe1...

[bors-servo]

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