Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext #11426

DDEFISHER · 2016-05-25T19:01:14Z

./mach build -d does not report any errors
./mach test-tidy --faster does not report any errors
These changes fix #11357
There are tests for these changes

This change is

highfive · 2016-05-25T19:01:19Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/webglrenderbuffer.rs, components/script/dom/webidls/WebGLRenderingContext.webidl, components/script/dom/webglframebuffer.rs, components/script/dom/webglbuffer.rs, components/script/dom/webglrenderingcontext.rs
@emilio: components/script/dom/webglrenderbuffer.rs, components/script/dom/webglframebuffer.rs, components/script/dom/webglbuffer.rs, components/script/dom/webglrenderingcontext.rs

highfive · 2016-05-25T19:01:20Z

Warning

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

highfive · 2016-05-25T19:10:42Z

New code was committed to pull request.

emilio · 2016-05-25T19:58:21Z

Looks good to me, modulo those nits, let's see which tests are affected :-)

@bors-servo: try

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

Previously, highfive wrote…

New code was committed to pull request.

Reviewed 5 of 5 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

components/script/dom/webglframebuffer.rs, line 20 [r1] (raw file):

    webgl_object: WebGLObject,
    id: u32,
    target: Cell<Option<u32>>,

Please add a comment saying that this can only be gl::FRAMEBUFFER at the moment. I'd like to convert it into a bool, but WebGL2 allows having gl::{READ, WRITE}_FRAMEBUFFER, so probably not worth it.

components/script/dom/webglrenderbuffer.rs, line 31 [r1] (raw file):

            webgl_object: WebGLObject::new_inherited(),
            id: id,
            target: Cell::new(None),

Same here, a renderbuffer can only be bound to the constant gl::RENDERBUFFER. In this case it might make more sense to use a bool, or at least adding a comment explaining it.

Comments from Reviewable

bors-servo · 2016-05-25T19:58:24Z

⌛ Trying commit 1bd87f0 with merge 0b9c1b1...


        Auto merge of #11426 - DDEFISHER:master, r=<try>

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #11357 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11426)

bors-servo · 2016-05-25T20:31:29Z

💔 Test failed - mac-rel-wpt

highfive · 2016-05-25T20:31:31Z

  ▶ TIMEOUT [expected OK] /html/dom/reflection-embedded.html
  │ 
  │ thread &#39;thread &#39;http_loader for http://web-platform.test:8000/html/dom/test-valueOfhttp_loader for http://web-platform.test:8000/html/dom/test-valueOf&#39; panicked at &#39;could not initialize thread_rng: Too many open files (os error 24)&#39;, ../src/libstd/rand/mod.rs:159
  │ &#39; panicked at &#39;stack backtrace:
  │ could not initialize thread_rng: Too many open files (os error 24)&#39;, /Users/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.3.14/src/lib.rs:873
  │ thread &#39;http_loader for http://web-platform.test:8000/html/dom/test-valueOf&#39; panicked at &#39;could not initialize thread_rng: Too many open files (os error 24)&#39;, ../src/libstd/rand/mod.rs:159
  │    1:        0x109c677a8 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x109c6dcf5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x109c6d90e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x1099bb2f2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x109c54db2 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x109c6e2b6 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x109c566a8 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x109c6e729 - std::rand::thread_rng::THREAD_RNG_KEY::__init::h536e3917997a1628
  │    9:        0x109c553bc - std::rand::thread_rng::h789d00d1fce4f79b
  │   10:        0x10855064c - _&lt;collections..hash..map..RandomState as core..default..Default&gt;::default::ha847494db5044892
  │   11:        0x108597374 - net::http_loader::load::h4cdeddfa5dca9e93
  │   12:        0x1085938ac - std::panicking::try::call::h2f55588c0568c703
  │   13:        0x109c7147b - __rust_try
  │   14:        0x109c71415 - __rust_maybe_catch_panic
  │ thread &#39;http_loader for http://web-platform.test:8000/html/dom/test-valueOf&#39; panicked at &#39;could not initialize thread_rng: Too many open files (os error 24)&#39;, /Users/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.3.14/src/lib.rs:873
  │   15:        0x1085958b4 - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::ha5012a14880088fa
  │   16:        0x109c6cc58 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   17:     0x7fff8e158059 - _pthread_body
  │   18:     0x7fff8e157fd6 - _pthread_start
  │ thread &#39;http_loader for http://web-platform.test:8000/html/dom/test-valueOf&#39; panicked at &#39;could not initialize thread_rng: Too many open files (os error 24)&#39;, /Users/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.3.14/src/lib.rs:873
  │ stack backtrace:
  │ thread &#39;http_loader for http://web-platform.test:8000/html/dom/test-valueOf&#39; panicked at &#39;could not initialize thread_rng: Too many open files (os error 24)&#39;, /Users/servo/.cargo/registry/src/github.com-1ecc6299db9ec823/rand-0.3.14/src/lib.rs:873
  │    1:        0x109c677a8 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x109c6dcf5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x109c6d90e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x1099bb2f2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x109c54db2 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x109c6e2b6 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x109c566a8 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x109a33e1f - rand::thread_rng::THREAD_RNG_KEY::__init::h4e1387a2e4a21393
  │    9:        0x109a33c5c - rand::thread_rng::he394b446ab605489
  │   10:        0x109a32b42 - uuid::Uuid::new_v4::hc206acac9c7e580d
  │   11:        0x108598cad - net::http_loader::load::h4cdeddfa5dca9e93
  │   12:        0x1085938ac - std::panicking::try::call::h2f55588c0568c703
  │   13:        0x109c7147b - __rust_try
  │   14:        0x109c71415 - __rust_maybe_catch_panic
  │   15:        0x1085958b4 - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::ha5012a14880088fa
  │   16:        0x109c6cc58 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   17:     0x7fff8e158059 - _pthread_body
  │   18:     0x7fff8e157fd6 - _pthread_start
  │ stack backtrace:
  │    1:        0x109c677a8 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x109c6dcf5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x109c6d90e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x1099bb2f2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x109c54db2 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x109c6e2b6 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x109c566a8 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x109c6e729 - std::rand::thread_rng::THREAD_RNG_KEY::__init::h536e3917997a1628
  │    9:        0x109c553bc - std::rand::thread_rng::h789d00d1fce4f79b
  │   10:        0x10855064c - _&lt;collections..hash..map..RandomState as core..default..Default&gt;::default::ha847494db5044892
  │   11:        0x108597374 - net::http_loader::load::h4cdeddfa5dca9e93
  │   12:        0x1085938ac - std::panicking::try::call::h2f55588c0568c703
  │   13:        0x109c7147b - __rust_try
  │   14:        0x109c71415 - __rust_maybe_catch_panic
  │   15:        0x1085958b4 - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::ha5012a14880088fa
  │   16:        0x109c6cc58 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   17:     0x7fff8e158059 - _pthread_body
  │   18:     0x7fff8e157fd6 - _pthread_start
  │ stack backtrace:
  │    1:        0x109c677a8 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x109c6dcf5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x109c6d90e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x1099bb2f2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x109c54db2 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x109c6e2b6 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x109c566a8 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x109a33e1f - rand::thread_rng::THREAD_RNG_KEY::__init::h4e1387a2e4a21393
  │    9:        0x109a33c5c - rand::thread_rng::he394b446ab605489
  │   10:        0x109a32b42 - uuid::Uuid::new_v4::hc206acac9c7e580d
  │   11:        0x108598cad - net::http_loader::load::h4cdeddfa5dca9e93
  │   12:        0x1085938ac - std::panicking::try::call::h2f55588</span><span class="stdout">c0568c703
  │   13:        0x109c7147b - __rust_try
  │   14:        0x109c71415 - __rust_maybe_catch_panic
  │   15:        0x1085958b4 - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::ha5012a14880088fa
  │   16:        0x109c6cc58 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   17:     0x7fff8e158059 - _pthread_body
  │   18:     0x7fff8e157fd6 - _pthread_start
  │ stack backtrace:
  │    1:        0x109c677a8 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x109c6dcf5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x109c6d90e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x1099bb2f2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x109c54db2 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x109c6e2b6 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x109c566a8 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x109a33e1f - rand::thread_rng::THREAD_RNG_KEY::__init::h4e1387a2e4a21393
  │    9:        0x109a33c5c - rand::thread_rng::he394b446ab605489
  │   10:        0x109a32b42 - uuid::Uuid::new_v4::hc206acac9c7e580d
  │   11:        0x108598cad - net::http_loader::load::h4cdeddfa5dca9e93
  │   12:        0x1085938ac - std::panicking::try::call::h2f55588c0568c703
  │   13:        0x109c7147b - __rust_try
  │   14:        0x109c71415 - __rust_maybe_catch_panic
  │   15:        0x1085958b4 - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::ha5012a14880088fa
  │   16:        0x109c6cc58 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   17:     0x7fff8e158059 - _pthread_body
  │   18:     0x7fff8e157fd6 - _pthread_start
  │ stack backtrace:
  │    1:        0x109c677a8 - std::sys::backtrace::tracing::imp::write::h9fb600083204ae7f
  │    2:        0x109c6dcf5 - std::panicking::default_hook::_$u7b$$u7b$closure$u7d$$u7d$::hca543c34f11229ac
  │    3:        0x109c6d90e - std::panicking::default_hook::hc2c969e7453d080c
  │    4:        0x1099bb2f2 - util::panicking::initiate_panic_hook::_$u7b$$u7b$closure$u7d$$u7d$::_$u7b$$u7b$closure$u7d$$u7d$::ha2bd86c312dc8d7a
  │    5:        0x109c54db2 - std::panicking::rust_panic_with_hook::hfe203e3083c2b544
  │    6:        0x109c6e2b6 - std::panicking::begin_panic::h4889569716505182
  │    7:        0x109c566a8 - std::panicking::begin_panic_fmt::h484cd47786497f03
  │    8:        0x109a33e1f - rand::thread_rng::THREAD_RNG_KEY::__init::h4e1387a2e4a21393
  │    9:        0x109a33c5c - rand::thread_rng::he394b446ab605489
  │   10:        0x109a32b42 - uuid::Uuid::new_v4::hc206acac9c7e580d
  │   11:        0x108598cad - net::http_loader::load::h4cdeddfa5dca9e93
  │   12:        0x1085938ac - std::panicking::try::call::h2f55588c0568c703
  │   13:        0x109c7147b - __rust_try
  │   14:        0x109c71415 - __rust_maybe_catch_panic
  │   15:        0x1085958b4 - _&lt;F as std..boxed..FnBox&lt;A&gt;&gt;::call_box::ha5012a14880088fa
  │   16:        0x109c6cc58 - std::sys::thread::Thread::new::thread_start::h6f266e069bf4ec2b
  │   17:     0x7fff8e158059 - _pthread_body
  └   18:     0x7fff8e157fd6 - _pthread_start

  ▶ Unexpected subtest result in /webgl/conformance-1.0.3/conformance/more/conformance/quickCheckAPI-B2.html:
  └ PASS [expected FAIL] WebGL test #0: testValidArgs

jdm · 2016-05-25T21:47:00Z

I thought reflection-embedded was disabled on mac now :<

highfive · 2016-05-27T04:35:23Z

New code was committed to pull request.

highfive · 2016-05-27T04:48:56Z

New code was committed to pull request.

emilio · 2016-05-29T00:17:36Z

Thanks for doing this!

It seems you'll need to update the test expectations for /webgl/conformance-1.0.3/conformance/more/conformance/quickCheckAPI-B2.html (that failed in the try run).

r=me with that and the nits addressed :)

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

Previously, highfive wrote…

New code was committed to pull request.

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

components/script/dom/webglframebuffer.rs, line 20 [r3] (raw file):

    webgl_object: WebGLObject,
    id: u32,
    ///target can only be gl::FRAMEBUFFER at the moment

nit: space after the comment start

components/script/dom/webglrenderbuffer.rs, line 20 [r3] (raw file):

    webgl_object: WebGLObject,
    id: u32,
    target: Cell<bool>,

can you rename this to ever_bound? It would feel kind of weird having this called target.

components/script/dom/webglrenderbuffer.rs, line 74 [r3] (raw file):

    }

    pub fn target(&self) -> bool {

ditto

Comments from Reviewable


        Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderi…

…ngContext

highfive · 2016-05-29T21:09:02Z

New code was committed to pull request.

DDEFISHER · 2016-05-29T21:26:39Z

@bors-servo: try

KiChjang · 2016-05-29T21:29:01Z

@bors-servo try

bors-servo · 2016-05-29T21:29:04Z

⌛ Trying commit c017438 with merge bdb3303...


        Auto merge of #11426 - DDEFISHER:master, r=<try>

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #11357 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11426)

bors-servo · 2016-05-29T22:18:30Z

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

emilio · 2016-05-30T01:03:54Z

Neat, thanks for doing this @DDEFISHER! :)

@bors-servo: r+

bors-servo · 2016-05-30T01:03:55Z

📌 Commit c017438 has been approved by emilio

bors-servo · 2016-05-30T01:03:56Z

⌛ Testing commit c017438 with merge b116489...


        Auto merge of #11426 - DDEFISHER:master, r=emilio

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy --faster` does not report any errors - [X] These changes fix #11357 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11426)

bors-servo · 2016-05-30T01:54:03Z

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

highfive assigned glennw May 25, 2016

emilio assigned emilio and unassigned glennw May 25, 2016

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderi…

Loading status checks…

c017438

…ngContext

bors-servo merged commit c017438 into servo:master May 30, 2016
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

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

homu Test successful
Details

servo / servo

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext #11426

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext #11426

DDEFISHER commented May 25, 2016 •

edited

highfive commented May 25, 2016

highfive commented May 25, 2016

highfive commented May 25, 2016

emilio commented May 25, 2016

bors-servo commented May 25, 2016

bors-servo commented May 25, 2016

highfive commented May 25, 2016

jdm commented May 25, 2016

highfive commented May 27, 2016

highfive commented May 27, 2016

emilio commented May 29, 2016

highfive commented May 29, 2016

DDEFISHER commented May 29, 2016

KiChjang commented May 29, 2016

bors-servo commented May 29, 2016

bors-servo commented May 29, 2016

emilio commented May 30, 2016

bors-servo commented May 30, 2016

bors-servo commented May 30, 2016

bors-servo commented May 30, 2016

servo / servo

Join GitHub today

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext #11426

Implement IsBuffer, IsFramebuffer and IsRenderbuffer for WebGLRenderingContext #11426

Conversation

DDEFISHER commented May 25, 2016 • edited

highfive commented May 25, 2016

highfive commented May 25, 2016

highfive commented May 25, 2016

emilio commented May 25, 2016

bors-servo commented May 25, 2016

bors-servo commented May 25, 2016

highfive commented May 25, 2016

jdm commented May 25, 2016

highfive commented May 27, 2016

highfive commented May 27, 2016

emilio commented May 29, 2016

highfive commented May 29, 2016

DDEFISHER commented May 29, 2016

KiChjang commented May 29, 2016

bors-servo commented May 29, 2016

bors-servo commented May 29, 2016

emilio commented May 30, 2016

bors-servo commented May 30, 2016

bors-servo commented May 30, 2016

bors-servo commented May 30, 2016

DDEFISHER commented May 25, 2016 •

edited