Implement GetVertexAttrib

[dzbarsky]

This change is

[highfive]

Heads up! This PR modifies the following files:

• @KiChjang: components/script/dom/webglrenderingcontext.rs, components/script/dom/webidls/WebGLRenderingContext.webidl
• @emilio: components/script/dom/webglrenderingcontext.rs

[highfive]

Warning

• These commits modify script code, but no tests are modified. Please consider adding a test!

[dzbarsky]

@emilio Thoughts on storing the 0'th vertex attrib in WebGLContext vs webrender_traits? The context seems to be where the state is so it makes sense to track there i guess.

Also some tests fail because it looks like VertexAttrib[1234]fv don't end up actually setting while VertexAttrib[1234]f do. Another thing we should track down.

[dzbarsky]

@emilio ok should be ready for review :)

[dzbarsky]

@bors-servo try

[bors-servo]

⌛ Trying commit dbf8915 with merge 17d15bc...

[dzbarsky]

this was a hack, we probably need to implement this somewhere right?

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

[emilio]

-S-awaiting-review +S-awaiting-answer

---

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

---

components/script/dom/webglrenderingcontext.rs, line 125 [r3] (raw file):
Yeah, https://github.com/servo/heapsize is the place to do that.

Not a huge deal (it has heapsize zero), but worth having.

---

components/script/dom/webglrenderingcontext.rs, line 1001 [r3] (raw file):
Probably not too difficult to check for constants::VERTEX_ATTRIB_ARRAY_BUFFER_BINDING and return the current bound_buffer_array?

---

components/script/dom/webglrenderingcontext.rs, line 1018 [r3] (raw file):
Instead of this ton of panics, it may be worth to have a wildcard like _ => unreachable!("Invalid parameter type for vertex attribute").

This applies to other sites where we use it.

---

tests/wpt/metadata/webgl/conformance-1.0.3/conformance/attribs/gl-vertex-attrib.html.ini, line 3 [r3] (raw file):
Are this tests still failing? They shouldn't fail, or at least with these messages, any clues on why?

---

Comments from Reviewable

[dzbarsky]

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

---

components/script/dom/webglrenderingcontext.rs, line 125 [r3] (raw file):
are you suggesting we can use VERTEX_ATTRIB_ARRAY_BUFFER_BINDING to avoid storing the 0'th vertex attrib? if so this will go away...

---

components/script/dom/webglrenderingcontext.rs, line 1001 [r3] (raw file):
sorry, what is VERTEX_ATTRIB_ARRAY_BUFFER_BINDING?

---

components/script/dom/webglrenderingcontext.rs, line 1018 [r3] (raw file):
We do this all over the place, let's fix in a followup?

---

tests/wpt/metadata/webgl/conformance-1.0.3/conformance/attribs/gl-vertex-attrib.html.ini, line 3 [r3] (raw file):
Yeah, it seems like the VertexAttrib vector methods are all busted and don't actually set things correctly - I debugged a little and it looks like going from JSObject to slice returns None but I haven't had time to figure out why yet

---

Comments from Reviewable

[emilio]

Looks good to me given the situation with the failing tests, let me know if you want to implement the constants::VERTEX_ATTRIB_ARRAY_BUFFER_BINDING thing (shouldn't be hard), and otherwise this is good to land.

Is the Buffer per-vertex or not? Not quite sure...
Let's land this and do VERTEX_ATTRIB_ARRAY_BUFFER_BINDING as a followup in any case

[highfive]

New code was committed to pull request.

[highfive]

New code was committed to pull request.

[dzbarsky]

@emilio Let's land this and do VERTEX_ATTRIB_ARRAY_BUFFER_BINDING as a followup

[nox]

@bors-servo r=emilio since he said "otherwise this is good to land".

@dzbarsky Please don't forget to do or file the follow-up. :)

[bors-servo]

📌 Commit 2bf016f has been approved by emilio

[bors-servo]

⌛ Testing commit 2bf016f with merge 710371a...

[bors-servo]

💔 Test failed - linux-rel

[highfive]

  ▶ TIMEOUT [expected FAIL] /css21_dev/html4/unicode-bidi-applies-to-008.htm
  │ 
  │ Xlib:  extension "XFree86-VidModeExtension" missing on display ":0".
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ thread 'Constellation' panicked at 'Couldn't receive FontCacheThread reply: IoError(Error { repr: Os { code: 104, message: "Connection reset by peer" } })', ../src/libcore/result.rs:785
  │ stack backtrace:
  │    1:     0x7f68bbbc3c50 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:     0x7f68bbbcb75b - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:     0x7f68bbbcb3c3 - std::panicking::default_hook::hc2c969e7453d080c
  │    4:     0x7f68bb3660b7 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::he2b22674ad1748f3
  │    5:     0x7f68bbbb171c - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:     0x7f68bbbcb9a1 - std::panicking::begin_panic::h4889569716505182
  │    7:     0x7f68bbbb311a - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:     0x7f68bbbcb93e - rust_begin_unwind
  │    9:     0x7f68bbc02a9f - core::panicking::panic_fmt::h257ceb0aa351d801
  │   10:     0x7f68bac6ab33 - core::result::unwrap_failed::h1d94443d858a91df
  │   11:     0x7f68bac9124a - gfx::font_cache_thread::FontCacheThread::exit::h335f05e55068200a
  │   12:     0x7f68ba1634d8 - _<constellation..Constellation<LTF, STF>>::handle_exit::h74520449cc783086
  │   13:     0x7f68ba138ecd - _<constellation..Constellation<LTF, STF>>::handle_request::h9204c5803595aa06
  │   14:     0x7f68ba12cadc - std::panicking::try::call::h9264c972d9469ce3
  │   15:     0x7f68bbbd604b - __rust_try
  │   16:     0x7f68bbbd5fee - __rust_maybe_catch_panic
  │   17:     0x7f68ba12e175 - _<F as std..boxed..FnBox<A>>::call_box::h3ac394cd6a44d799
  │   18:     0x7f68bbbc9bb4 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   19:     0x7f68b7b46181 - start_thread
  │   20:     0x7f68b765d47c - __clone
  └   21:                0x0 - <unknown>

[emilio]

@bors-servo: retry

• #8815

[bors-servo]

⌛ Testing commit 2bf016f with merge fe116b4...

[bors-servo]

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, windows

[nox]

@dzbarsky Do you remember why it's necessary to keep the value of vertex attrib 0 on the DOM side?

[dzbarsky]

When I first implemented this in 2bf016f GetVertexAttrib had a path to serve 0/CURRENT_VERTEX_ATTRIB from DOM side - I don't recall why I did this though...perhaps it was a fast path or something?

[dzbarsky]

Oh I see now that this is the original place we added it :) Most likely I was cargo-culting the pattern of retaining state like current_program, though this one was likely not necessary. Oops!

[nox]

Seems like calling glGetVertexAttrib* with index 0 causes issues on some drivers.

[nox]

https://www.khronos.org/webgl/wiki/WebGL_and_OpenGL_Differences#Vertex_Attribute_0