Add WebGLContextAttributes support #6183

emilio · 2015-05-25T23:24:07Z

I couldn't add the getContextAttributes method since CodegenRust
doesn't know how to return a dictionary value, I'll take a look at it ASAP.

I think the helper functions can return directly the renderer, since they're used just for that, but I wanted to hear your opinions about this.

By the way I'm interested in adding more serious tests for WebGL, and I think the khronos conformance suit should be the best option.

Should I try to integrate it in wpt, or making a tests/webgl directory (or similar) inside the servo tree? (Maybe this question should be for @Ms2ger)

hoppipolla-critic-bot · 2015-05-25T23:24:12Z

Critic review: https://critic.hoppipolla.co.uk/r/5098

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 git rebase -i or git commit --amend) when updating this pull request.

emilio · 2015-05-26T01:41:21Z

I added to codegen the posibility to return a dictionary value.

I think my changes are correct (converting to a plain JS dictionary should suffice since it does not need to be rootable), and it definitely works (see the getContextAttributes test) but I'm not totally sure.

I kept commits split, just in case :P

nox · 2015-05-26T21:30:54Z

Review status: all files reviewed, 16 unresolved discussions, all commit checks successful.
Reviewed files:

components/canvas/webgl_paint_task.rs @ r2
components/canvas_traits/Cargo.toml @ r2
components/canvas_traits/lib.rs @ r2
components/script/Cargo.toml @ r1
components/script/dom/bindings/codegen/CodegenRust.py @ r3
components/script/dom/bindings/utils.rs @ r3
components/script/dom/htmlcanvaselement.rs @ r1
components/script/dom/webglrenderingcontext.rs @ r2
components/script/dom/webidls/HTMLCanvasElement.webidl @ r1
components/script/dom/webidls/WebGLRenderingContext.webidl @ r2
components/script/lib.rs @ r1
components/servo/Cargo.lock @ r2
ports/cef/Cargo.lock @ r3
ports/gonk/Cargo.lock @ r3
tests/html/test_webgl_context_attributes.html @ r2

^{components/script/dom/bindings/codegen/CodegenRust.py, line 4591 [r3] (raw file):}
indentLevel=8

^{components/script/dom/htmlcanvaselement.rs, line 139 [r3] (raw file):}
I'm pretty sure you don't need extended_deref().

^{components/script/dom/htmlcanvaselement.rs, line 143 [r3] (raw file):}
You can just use Temporary::from_rooted on the JS value.

^{components/script/dom/htmlcanvaselement.rs, line 148 [r3] (raw file):}
Same remark about extended_deref().

^{components/script/dom/htmlcanvaselement.rs, line 152 [r3] (raw file):}
Same remark about Temporary::from_rooted().

^{components/script/dom/htmlcanvaselement.rs, line 221 [r3] (raw file):}
The WebGL spec says " If getContext() was invoked with a second argument, options, set the attributes of contextAttributes from those specified in options." and nothing about what the second argument should be, type-wise.

@jdm?

^{components/script/dom/htmlcanvaselement.rs, line 242 [r3] (raw file):}
CodegenRust almost generates all of this for you in WebGLContextAttributes::new(), but if any one of these values is not a boolean, the function will return an error. Such a shame that the WebGL specification isn't very clear on what to do in such a case.

If this is required, I would just make it a bare function in this module instead of a trait.

@jdm?

^{components/script/dom/htmlcanvaselement.rs, line 283 [r3] (raw file):}
What would be the problem with supporting preserve_drawing_buffer right now, given it's already known to GLContextAttributes?

^{components/script/dom/webidls/HTMLCanvasElement.webidl, line 15 [r3] (raw file):}
The spec says "RenderingContext? getContext(DOMString contextId, any... arguments)", I guess Servo doesn't handle variadic any arguments?

^{components/servo/Cargo.lock, line 537 [r3] (raw file):}
I'm not sure I understand why html5ever was updated in this PR.

^{components/servo/Cargo.lock, line 552 [r3] (raw file):}
Same remark about html5ever.

^{ports/cef/Cargo.lock, line 539 [r3] (raw file):}
Same remark about html5ever.

^{ports/cef/Cargo.lock, line 554 [r3] (raw file):}
Same remark about html5ever.

^{ports/gonk/Cargo.lock, line 447 [r3] (raw file):}
Same remark about html5ever.

^{ports/gonk/Cargo.lock, line 462 [r3] (raw file):}
Same remark about html5ever.

Comments from the review on Reviewable.io

emilio · 2015-05-26T22:52:47Z

Review status: 9 of 15 files reviewed, 7 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/codegen/CodegenRust.py, line 4591 [r3] (raw file):}
Good catch! (3 a.m. code, sorry...)

^{components/script/dom/htmlcanvaselement.rs, line 139 [r3] (raw file):}
Didn't realise it automatically implemented the Deref trait, my bad.

^{components/script/dom/htmlcanvaselement.rs, line 221 [r3] (raw file):}
See the next comment, since it's related.

We can either return None or a context with the default attributes when the second argument is not an object. Right now I'm returning the default attributes, but...

^{components/script/dom/htmlcanvaselement.rs, line 242 [r3] (raw file):}
It seems that both Firefox and Chromium cast each value, if the second argument is an object:

var gl = document.createElement('canvas').getContext('webgl', { alpha: '' });
gl.getContextAttributes().alpha === false

But apart from that the behaviour is inconsistent. In Firefox:

canvas.getContext('webgl', 'literal string'); // null
canvas.getContext('webgl', function() {}); // WebGLRenderingContext with default attributes

While in Chromium both return a rendering context with default parameters.

I couldn't test in more browsers.

That being said, it seems to me (after reading components/script/dom/bindings/conversions.rs) that the value is casted if it's not a boolean (JS_ValueToBoolean, so this is definitely not necessary.

^{components/script/dom/htmlcanvaselement.rs, line 283 [r3] (raw file):}
It's known to GLContextAttributes, but it actually has no effect, since there's no support for it (yet).

You're right tough, so I released an update which prevents creating contexts with preserve_drawing_buffer.

^{components/script/dom/webidls/HTMLCanvasElement.webidl, line 15 [r3] (raw file):}
Nope. Firefox also does this though.

^{components/servo/Cargo.lock, line 537 [r3] (raw file):}
My fault, didn't realize rust was upgraded to 1.1.1 and I was using system rust. Reverted

Comments from the review on Reviewable.io

nox · 2015-05-27T07:13:46Z

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/bindings/codegen/CodegenRust.py @ r4
components/script/dom/htmlcanvaselement.rs @ r5
components/servo/Cargo.lock @ r4
ports/cef/Cargo.lock @ r4
ports/gonk/Cargo.lock @ r4
tests/html/test_webgl_context_attributes.html @ r5

^{components/script/dom/htmlcanvaselement.rs, line 242 [r3] (raw file):}
Pinging @jdm to add a pair of balls on this issue, otherwise it looks OK to me.

^{components/script/dom/htmlcanvaselement.rs, line 286 [r5] (raw file):}
Maybe GLContextAttributes::from_webgl()?

^{components/script/dom/webidls/HTMLCanvasElement.webidl, line 15 [r3] (raw file):}
"Nope" as in "Nope it doesn't support them"?

Pinging @jdm.

Comments from the review on Reviewable.io

emilio · 2015-05-27T07:45:09Z

Review status: all files reviewed, 4 unresolved discussions, all commit checks successful.

^{components/script/dom/htmlcanvaselement.rs, line 139 [r3] (raw file):}
Done.

^{components/script/dom/htmlcanvaselement.rs, line 286 [r5] (raw file):}
What about using the standard From trait?

^{components/script/dom/webidls/HTMLCanvasElement.webidl, line 15 [r3] (raw file):}
My bad, I was thinking of Codegen.py not CodegenRust.py. It's supported in CodegenRust.

Anyways I think as of right now this is way cleaner.

Comments from the review on Reviewable.io

jdm · 2015-05-27T13:26:28Z

Review status: all files reviewed, 4 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlcanvaselement.rs, line 233 [r3] (raw file):}
s/balls/eyes/ please. This looks OK to me too.

^{components/script/dom/webidls/HTMLCanvasElement.webidl, line 15 [r3] (raw file):}
Let's follow the spec when possible.

Comments from the review on Reviewable.io

bors-servo · 2015-05-27T19:57:59Z

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

nox · 2015-05-27T21:07:30Z

Review status: all files reviewed, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlcanvaselement.rs, line 233 [r3] (raw file):}
I swear to cow that I meant to type eyeballs there, and not this insane double entendre. What the hell happened here, I will never know.

^{components/script/dom/htmlcanvaselement.rs, line 286 [r5] (raw file):}
That would be perfect I think.

Comments from the review on Reviewable.io

emilio · 2015-05-27T23:32:12Z

Review status: 6 of 15 files reviewed, 4 unresolved discussions, all commit checks successful.

^{components/script/dom/htmlcanvaselement.rs, line 286 [r5] (raw file):}
Done.

^{components/script/dom/webidls/HTMLCanvasElement.webidl, line 15 [r3] (raw file):}
Done.

Comments from the review on Reviewable.io

nox · 2015-05-28T08:36:43Z

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/Cargo.toml @ r6
components/script/dom/bindings/codegen/CodegenRust.py @ r6
components/script/dom/bindings/utils.rs @ r6
components/script/dom/htmlcanvaselement.rs @ r6
components/script/dom/webidls/HTMLCanvasElement.webidl @ r6
components/servo/Cargo.lock @ r6
ports/cef/Cargo.lock @ r6
ports/gonk/Cargo.lock @ r6
tests/html/test_webgl_context_attributes.html @ r7

^{components/script/dom/htmlcanvaselement.rs, line 287 [r7] (raw file):}
I'm not sure this comment is useful, no one is confused about "impl<'a> From<&'a str> for String".

Otherwise, reference*.

Comments from the review on Reviewable.io

emilio · 2015-05-28T09:46:56Z

Review status: 14 of 15 files reviewed, 4 unresolved discussions, some commit checks pending.

^{components/script/dom/htmlcanvaselement.rs, line 287 [r7] (raw file):}
Yep, that was a typo.

The comment was just to justify not using:

impl From<WebGLContextAttributes> for ...

Anyways, removed :P

Comments from the review on Reviewable.io

nox · 2015-05-28T10:11:42Z

Review status: all files reviewed, 2 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/htmlcanvaselement.rs @ r8

Comments from the review on Reviewable.io

nox · 2015-05-28T10:16:11Z

@ecoal95 Squash everything and it can be merged.

emilio · 2015-05-28T10:28:35Z

Done :)

Manishearth · 2015-05-28T10:36:07Z

@bors-servo: r+

bors-servo · 2015-05-28T10:36:08Z

📌 Commit cac4c5d has been approved by Manishearth

bors-servo · 2015-05-28T10:54:09Z

⌛ Testing commit cac4c5d with merge 4a99020...


        Auto merge of #6183 - ecoal95:webglcontextattributes, r=Manishearth

@jdm

r? @jdm I couldn't add the `getContextAttributes` method since `CodegenRust` doesn't know how to return a dictionary value, I'll take a look at it ASAP. I think the helper functions can return directly the renderer, since they're used just for that, but I wanted to hear your opinions about this. By the way I'm interested in adding more serious tests for WebGL, and I think the [khronos conformance suit](https://github.com/KhronosGroup/WebGL/tree/master/conformance-suites/1.0.3) should be the best option. Should I try to integrate it in wpt, or making a `tests/webgl` directory (or similar) inside the servo tree? (Maybe this question should be for @Ms2ger)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6183)

bors-servo · 2015-05-28T11:23:58Z

💔 Test failed - mac1

Manishearth · 2015-05-28T11:26:23Z


Unexpected Results
==================

/2dcontext/compositing/2d.composite.uncovered.nocontext.copy.html
-----------------------------------------------------------------
TIMEOUT [Parent]
/2dcontext/compositing/2d.composite.uncovered.nocontext.destination-atop.html
-----------------------------------------------------------------------------
TIMEOUT [Parent]
/2dcontext/compositing/2d.composite.uncovered.nocontext.destination-in.html
---------------------------------------------------------------------------
TIMEOUT [Parent]
/2dcontext/compositing/2d.composite.uncovered.nocontext.source-in.html
----------------------------------------------------------------------
TIMEOUT [Parent]
/2dcontext/compositing/2d.composite.uncovered.nocontext.source-out.html
-----------------------------------------------------------------------
TIMEOUT [Parent]
16:43.01 LOG: MainThread INFO Closing logging queue
16:43.01 LOG: MainThread INFO queue closed
program finished with exit code 1
elapsedTime=1011.482173

nox · 2015-05-28T11:34:54Z

Review status: all files reviewed, 3 unresolved discussions, some commit checks failed.

^{components/script/dom/htmlcanvaselement.rs, line 140 [r9] (raw file):}
While at it, maybe context_2d and context_webgl should be merged into an enum type?

Comments from the review on Reviewable.io

emilio · 2015-05-28T20:58:12Z

Review status: 14 of 16 files reviewed, 4 unresolved discussions, some commit checks pending.

^{components/script/dom/htmlcanvaselement.rs, line 140 [r9] (raw file):}
I've struggled a bit with this. I've (unsuccessfully) tried making an union like this:

#[jstraceable]
pub enum CanvasContext {
    Context2d(MutHeap<JS<CanvasRenderingContext2D>>),
    WebGL(MutHeap<JS<WebGLRenderingContext>>),
    None,
}

I can't do that since we need interior mutability. I can't wrap it in a Cell (since MutHeap doesn't implement Copy), but I also can't wrap an enum inside a MutNullableHeap since the it won't implement HeapGCValue...

I've been able to do it, passing the web platform tests without errors, manually implementing HeapGCValue and the needed traits for the union below... Is this what we want or will it bring any unexpected side effect?

#[jstraceable]
#[must_root]
#[derive(Clone, Copy)]
pub enum CanvasContext {
    Context2d(JS<CanvasRenderingContext2D>),
    WebGL(JS<WebGLRenderingContext>), 
}   

impl HeapGCValue for CanvasContext {}

#[dom_struct]
pub struct HTMLCanvasElement {
    // ...
    context: MutNullableHeap<CanvasContext>,
    // ...
}

I've pushed, but if we want to roll back it's easy.

Comments from the review on Reviewable.io

nox · 2015-05-30T10:32:30Z

Review status: all files reviewed, 2 unresolved discussions, all commit checks successful.
Reviewed files:

components/script/dom/canvasrenderingcontext2d.rs @ r10
components/script/dom/htmlcanvaselement.rs @ r10

^{components/script/dom/htmlcanvaselement.rs, line 140 [r9] (raw file):}
Sounds good to me, @jdm?

Comments from the review on Reviewable.io

jdm · 2015-05-30T19:42:42Z

Review status: all files reviewed, 3 unresolved discussions, all commit checks successful.

^{components/script/dom/htmlcanvaselement.rs, line 151 [r9] (raw file):}
Agreed.

Comments from the review on Reviewable.io

emilio · 2015-06-01T13:27:17Z

Is there something left to do about this one?


        Add WebGLContextAttributes support

This commit also: * Allows to return non-rootable dictionaries from Codegen. * Merges the two context types in an enum type.

nox · 2015-06-01T13:30:11Z

I don't think so.

nox · 2015-06-01T13:30:34Z

@bors-servo: r+

emilio · 2015-06-01T13:31:10Z

Was squashing right now

nox · 2015-06-01T13:31:41Z

Ok, squash it then. The bot doesn't know I'm a reviewer yet. :)

emilio · 2015-06-01T13:32:50Z

Done :P

@jdm must be happy about having a bit more help ;)

nox · 2015-06-01T13:37:45Z

@bors-servo: r+

bors-servo · 2015-06-01T13:37:45Z

📌 Commit b3ac346 has been approved by nox

bors-servo · 2015-06-01T13:37:48Z

⌛ Testing commit b3ac346 with merge 0de09b9...


        Auto merge of #6183 - ecoal95:webglcontextattributes, r=nox

@jdm

r? @jdm I couldn't add the `getContextAttributes` method since `CodegenRust` doesn't know how to return a dictionary value, I'll take a look at it ASAP. I think the helper functions can return directly the renderer, since they're used just for that, but I wanted to hear your opinions about this. By the way I'm interested in adding more serious tests for WebGL, and I think the [khronos conformance suit](https://github.com/KhronosGroup/WebGL/tree/master/conformance-suites/1.0.3) should be the best option. Should I try to integrate it in wpt, or making a `tests/webgl` directory (or similar) inside the servo tree? (Maybe this question should be for @Ms2ger)  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6183)

bors-servo · 2015-06-01T14:21:51Z

☀️ Test successful - android, gonk, linux1, linux2, mac1, mac2

highfive added the S-awaiting-review label May 25, 2015

emilio force-pushed the emilio:webglcontextattributes branch 2 times, most recently from eda4b63 to 1a375ad May 25, 2015

emilio force-pushed the emilio:webglcontextattributes branch 2 times, most recently from 33c833a to b27a672 May 27, 2015

emilio force-pushed the emilio:webglcontextattributes branch from b27a672 to 25dd4b7 May 27, 2015

emilio force-pushed the emilio:webglcontextattributes branch from 7a777d2 to cac4c5d May 28, 2015

nox mentioned this pull request May 28, 2015

Integrate Khronos' WebGL conformance test suite #6207

Closed

Manishearth added the S-tests-failed label May 28, 2015

highfive added S-awaiting-review and removed S-tests-failed labels May 28, 2015

emilio force-pushed the emilio:webglcontextattributes branch from a1dee1e to fdc3a59 May 28, 2015

nox added S-awaiting-answer and removed S-awaiting-review labels May 30, 2015

Manishearth added S-awaiting-review and removed S-awaiting-answer labels Jun 1, 2015

Add WebGLContextAttributes support

Loading status checks…

b3ac346

This commit also: * Allows to return non-rootable dictionaries from Codegen. * Merges the two context types in an enum type.

emilio force-pushed the emilio:webglcontextattributes branch from fdc3a59 to b3ac346 Jun 1, 2015

jdm added S-awaiting-merge and removed S-awaiting-review labels Jun 1, 2015

bors-servo merged commit b3ac346 into servo:master Jun 1, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

This was referenced Jun 1, 2015

WebGLRenderingContext getters and `getParameter` #6240

Merged

Add support for switching frames with the webdriver API. #6009

Merged

emilio deleted the emilio:webglcontextattributes branch Jun 1, 2015

servo / servo

Add WebGLContextAttributes support #6183

Add WebGLContextAttributes support #6183

emilio commented May 25, 2015

hoppipolla-critic-bot commented May 25, 2015

emilio commented May 26, 2015

nox commented May 26, 2015

emilio commented May 26, 2015

nox commented May 27, 2015

emilio commented May 27, 2015

jdm commented May 27, 2015

bors-servo commented May 27, 2015

nox commented May 27, 2015

emilio commented May 27, 2015

nox commented May 28, 2015

emilio commented May 28, 2015

nox commented May 28, 2015

nox commented May 28, 2015

emilio commented May 28, 2015

Manishearth commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

Manishearth commented May 28, 2015

nox commented May 28, 2015

emilio commented May 28, 2015

nox commented May 30, 2015

jdm commented May 30, 2015

emilio commented Jun 1, 2015

nox commented Jun 1, 2015

nox commented Jun 1, 2015

emilio commented Jun 1, 2015

nox commented Jun 1, 2015

emilio commented Jun 1, 2015

nox commented Jun 1, 2015

bors-servo commented Jun 1, 2015

bors-servo commented Jun 1, 2015

bors-servo commented Jun 1, 2015

servo / servo

Join GitHub today

Add WebGLContextAttributes support #6183

Add WebGLContextAttributes support #6183

Conversation

emilio commented May 25, 2015

hoppipolla-critic-bot commented May 25, 2015

emilio commented May 26, 2015

nox commented May 26, 2015

emilio commented May 26, 2015

nox commented May 27, 2015

emilio commented May 27, 2015

jdm commented May 27, 2015

bors-servo commented May 27, 2015

nox commented May 27, 2015

emilio commented May 27, 2015

nox commented May 28, 2015

emilio commented May 28, 2015

nox commented May 28, 2015

nox commented May 28, 2015

emilio commented May 28, 2015

Manishearth commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

bors-servo commented May 28, 2015

Manishearth commented May 28, 2015

nox commented May 28, 2015

emilio commented May 28, 2015

nox commented May 30, 2015

jdm commented May 30, 2015

emilio commented Jun 1, 2015

nox commented Jun 1, 2015

nox commented Jun 1, 2015

emilio commented Jun 1, 2015

nox commented Jun 1, 2015

emilio commented Jun 1, 2015

nox commented Jun 1, 2015

bors-servo commented Jun 1, 2015

bors-servo commented Jun 1, 2015

bors-servo commented Jun 1, 2015