Rewrite WebGLRenderingContext to use typed array APIs #15352

jdm · 2017-02-02T17:04:10Z

Rewrite WebGLRenderingContext to use typed array APIs. Based on #15267.

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

This change is

jdm · 2017-02-02T17:04:20Z

@bors-servo: r+

bors-servo · 2017-02-02T17:04:21Z

📌 Commit 6b34010 has been approved by jdm

jdm · 2017-02-02T17:04:32Z

@bors-servo: r-

jdm · 2017-02-02T17:06:48Z

@nox, could you review the second commit?

emilio

You can land the second commit with r=emilio with that comment added :)

emilio · 2017-02-02T17:21:49Z

components/script/dom/webglrenderingcontext.rs

 {
+    let mut typed_array_root = Rooted::new_unrooted();
+    let typed_array: Option<TypedArray<T>> =


Could you add a TODO or similar pointing to servo/rust-mozjs#330?

emilio · 2017-02-02T17:21:49Z

components/script/dom/webglrenderingcontext.rs

+                                            -> Result<Vec<T::Element>, Error>
+    where T: TypedArrayElement,
+          T::Element: FromJSValConvertible + Clone,
+          <T::Element as FromJSValConvertible>::Config: Clone,


You could have also done just T::Element: FromJSValConvertible<Config = ()> + Clone I think.

emilio · 2017-02-02T17:22:25Z

components/script/dom/bindings/conversions.rs

-
-/// Similar API as the array_buffer_view_xxx functions, but for ArrayBuffer
-/// objects.
-pub unsafe fn array_buffer_data<'a, T>(ab: *mut JSObject) -> Option<&'a mut [T]>


Nice to see this going away btw :)

jdm · 2017-02-02T17:33:46Z

@bors-servo: r=jdm,emilio

bors-servo · 2017-02-02T17:33:47Z

📌 Commit d96f18c has been approved by jdm,emilio

bors-servo · 2017-02-02T17:33:51Z

⌛ Testing commit d96f18c with merge 6b72907...


        Auto merge of #15352 - jdm:Rewrite-WebGLRenderingContext-to-use-typed…

…-array-APIs, r=jdm,emilio Rewrite WebGLRenderingContext to use typed array APIs Rewrite WebGLRenderingContext to use typed array APIs. Based on #15267. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14676 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15352)

bors-servo · 2017-02-02T18:12:55Z

💔 Test failed - linux-rel-wpt

emilio · 2017-02-02T18:17:31Z

Those seem pretty legit: They're segfaults under SpiderMonkey:

  ▶ CRASH [expected OK] /webgl/conformance-1.0.3/conformance/textures/texture-clear.html
  │ 
  │ VMware, Inc.
  │ Gallium 0.4 on softpipe
  │ 3.3 (Core Profile) Mesa 12.0.1
  │ DESCRIPTION: WebGL texture clear conformance test.
  │ PASS getError was expected value: NO_ERROR : Should be no errors from setup.
  │ Stack trace for thread "ScriptThread PipelineId { namespace_id: PipelineNamespaceId(0), index: PipelineIndex(1) }"
  │ stack backtrace:
  │    0:     0x7f3e5ccec74d - backtrace::backtrace::trace::hec2e63bb08ff5a5e
  │    1:     0x7f3e5ccecc32 - backtrace::capture::Backtrace::new::h312dcbed970dcedf
  │    2:     0x7f3e5b7a8054 - servo::install_crash_handler::handler::h16a97a4726f3da56
  │    3:     0x7f3e5ca04463 - AsmJSFaultHandler
  │                         at /home/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7cd72d8/mozjs/js/src/asmjs/WasmSignalHandlers.cpp:1171
  │    4:     0x7f3e5967e32f - <unknown>
  │    5:     0x7f3e5c66db58 - GetObjectClass
  │                         at /home/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7cd72d8/mozjs/js/src/jsfriendapi.h:632
  │                          - IsProxy
  │                         at /home/servo/buildbot/slave/linux-rel-wpt/build/target/release/build/mozjs_sys-b75999929c5a0ce6/out/dist/include/js/Proxy.h:354
  │                          - IsWrapper
  │                         at /home/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7cd72d8/mozjs/js/src/jswrapper.h:337
  │                          - is<js::WrapperObject>
  │                         at /home/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7cd72d8/mozjs/js/src/vm/WrapperObject.h:35
  │                          - _ZN2js16UnwrapOneCheckedEP8JSObjectb
  │                         at /home/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7cd72d8/mozjs/js/src/proxy/Wrapper.cpp:372
  │    6:     0x7f3e5c66dbe1 - _ZN2js13CheckedUnwrapEP8JSObjectb
  │                         at /home/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7cd72d8/mozjs/js/src/proxy/Wrapper.cpp:363
  │    7:     0x7f3e5c792c8d - _ZN2js16UnwrapUint8ArrayEP8JSObject
  │                         at /home/servo/.cargo/git/checkouts/mozjs-fa11ffc7d4f1cc2d/7cd72d8/mozjs/js/src/vm/TypedArrayObject.cpp:2144
  │    8:     0x7f3e5c03e2c4 - script::dom::webglrenderingcontext::WebGLRenderingContext::validate_tex_image_2d_data::h631addfd590d2687
  │    9:     0x7f3e5c05b743 - <script::dom::webglrenderingcontext::WebGLRenderingContext as script::dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLRenderingContextBinding::WebGLRenderingContextMethods>::TexImage2D::h9961e16fd2832929
  │   10:     0x7f3e5bc85357 - std::panicking::try::do_call::h95ab72bcfb317701
  │   11:     0x7f3e5d64382a - panic_unwind::__rust_maybe_catch_panic
  │                         at /buildslave/rust-buildbot/slave/nightly-dist-rustc-linux/build/src/libpanic_unwind/lib.rs:98
  │   12:     0x7f3e5c357c1b - script::dom::bindings::codegen::Bindings::WebGLRenderingContextBinding::WebGLRenderingContextBinding::texImage2D::h52ab0604cb1e42c8
  │   13:     0x7f3e5c3b4c84 - CallJitMethodOp
  │   14:     0x7f3e5be7c8a4 - script::dom::bindings::utils::generic_call::h5b62bb2904f73372
  │   15:     0x7f3e5c710a90 - CallJSNative

jdm · 2017-02-02T19:16:08Z

Filed servo/rust-mozjs#332. We'll wait until it's solved before merging these changes again.

bors-servo · 2017-02-03T15:54:34Z

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

jdm · 2017-02-06T21:12:42Z

No longer blocked! servo/rust-mozjs#332 is fixed, so this PR should update js to the latest version using ./mach cargo-update -p js.


        Use TypedArray


        Make typed_array_or_sequence_to_vec support typed arrays.

jdm · 2017-02-10T21:34:54Z

@bors-servo: r+

bors-servo · 2017-02-10T21:34:54Z

📌 Commit baf8c54 has been approved by jdm

bors-servo · 2017-02-10T21:57:37Z

⌛ Testing commit baf8c54 with merge 62ddc62...


        Auto merge of #15352 - jdm:Rewrite-WebGLRenderingContext-to-use-typed…

…-array-APIs, r=jdm Rewrite WebGLRenderingContext to use typed array APIs Rewrite WebGLRenderingContext to use typed array APIs. Based on #15267. --- - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14676 - [X] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15352)

bors-servo · 2017-02-10T22:42:28Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: jdm
Pushing 62ddc62 to master...

highfive assigned wafflespeanut Feb 2, 2017

highfive added the S-awaiting-review label Feb 2, 2017

highfive assigned jdm and unassigned wafflespeanut Feb 2, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 2, 2017

jdm mentioned this pull request Feb 2, 2017

Rewrite WebGLRenderingContext to use typed array APIs #15267

Closed

3 of 3 tasks complete

jdm assigned nox and unassigned jdm Feb 2, 2017

jdm added S-awaiting-review and removed S-awaiting-merge labels Feb 2, 2017

emilio approved these changes Feb 2, 2017

View changes

emilio reviewed Feb 2, 2017

View changes

emilio changed the title ~~Rewrite web gl rendering context to use typed array ap is~~ Rewrite WebGLRenderingContext to use typed array APIs Feb 2, 2017

jdm force-pushed the jdm:Rewrite-WebGLRenderingContext-to-use-typed-array-APIs branch from 6b34010 to d96f18c Feb 2, 2017

highfive assigned jdm and unassigned nox Feb 2, 2017

highfive added S-awaiting-merge and removed S-awaiting-review labels Feb 2, 2017

highfive added S-tests-failed and removed S-awaiting-merge labels Feb 2, 2017

jdm added the S-blocked-on-external label Feb 2, 2017

highfive added the S-needs-rebase label Feb 3, 2017

jdm removed the S-blocked-on-external label Feb 6, 2017

jdm added the S-needs-code-changes label Feb 6, 2017

yysung1123 and others added 2 commits Jan 23, 2017

Use TypedArray

6c4ce72

Make typed_array_or_sequence_to_vec support typed arrays.

Loading status checks…

baf8c54

jdm force-pushed the jdm:Rewrite-WebGLRenderingContext-to-use-typed-array-APIs branch from d96f18c to baf8c54 Feb 10, 2017

highfive added S-awaiting-review and removed S-tests-failed S-needs-code-changes labels Feb 10, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-rebase labels Feb 10, 2017

bors-servo merged commit baf8c54 into servo:master Feb 10, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

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

homu Test successful
Details

highfive removed the S-awaiting-merge label Feb 10, 2017

servo / servo

Rewrite WebGLRenderingContext to use typed array APIs #15352

Rewrite WebGLRenderingContext to use typed array APIs #15352

jdm commented Feb 2, 2017 •

edited by larsbergstrom

jdm commented Feb 2, 2017

bors-servo commented Feb 2, 2017

jdm commented Feb 2, 2017

jdm commented Feb 2, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

emilio commented Feb 2, 2017

jdm commented Feb 2, 2017

bors-servo commented Feb 3, 2017

jdm commented Feb 6, 2017

jdm commented Feb 10, 2017

bors-servo commented Feb 10, 2017

bors-servo commented Feb 10, 2017

bors-servo commented Feb 10, 2017

servo / servo

Join GitHub today

Rewrite WebGLRenderingContext to use typed array APIs #15352

Rewrite WebGLRenderingContext to use typed array APIs #15352

Conversation

jdm commented Feb 2, 2017 • edited by larsbergstrom

jdm commented Feb 2, 2017

bors-servo commented Feb 2, 2017

jdm commented Feb 2, 2017

jdm commented Feb 2, 2017

emilio left a comment

This comment has been minimized.

emilio Feb 2, 2017

This comment has been minimized.

emilio Feb 2, 2017

This comment has been minimized.

emilio Feb 2, 2017

jdm commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

bors-servo commented Feb 2, 2017

emilio commented Feb 2, 2017

jdm commented Feb 2, 2017

bors-servo commented Feb 3, 2017

jdm commented Feb 6, 2017

jdm commented Feb 10, 2017

bors-servo commented Feb 10, 2017

bors-servo commented Feb 10, 2017

bors-servo commented Feb 10, 2017

jdm commented Feb 2, 2017 •

edited by larsbergstrom