Remove slice_to_array_buffer_view and update_array_buffer_view #15427

yysung1123 · 2017-02-07T19:08:55Z

Remove slice_to_array_buffer_view and update_array_buffer_view

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15350
These changes do not require tests

This change is

highfive · 2017-02-07T19:08:59Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/vreyeparameters.rs, components/script/dom/vrframedata.rs, components/script/dom/vrpose.rs, components/script/dom/bindings/conversions.rs, components/script/dom/vrstageparameters.rs
@KiChjang: components/script/dom/vreyeparameters.rs, components/script/dom/vrframedata.rs, components/script/dom/vrpose.rs, components/script/dom/bindings/conversions.rs, components/script/dom/vrstageparameters.rs

highfive · 2017-02-07T19:09:00Z

Warning

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

jdm · 2017-02-07T19:10:28Z

components/script/dom/vrframedata.rs

+            let _ = TypedArray::<Float32>::create(global.get_cx(), matrix.len() as u32, Some(&matrix),
+                MutableHandleObject::from_marked_location(framedata.right_proj.get_unsafe()));
+            let _ = TypedArray::<Float32>::create(global.get_cx(), matrix.len() as u32, Some(&matrix),
+                MutableHandleObject::from_marked_location(framedata.right_view.get_unsafe()));


We should add a handle_mut() method to Heap instead of using from_marked_location everywhere.

Yes, the code is too long.
I can add handle_mut() and fix the code in this PR.

yysung1123 · 2017-02-08T18:38:28Z

Change all MutableHandleObject::from_marked_location to handle_mut().

jdm

Looks good! I just have some stylistic comments to make it easier to read.

jdm · 2017-02-09T19:17:08Z

components/script/dom/vreyeparameters.rs

@@ -39,7 +39,8 @@ impl VREyeParameters {
        };

        unsafe {
-            result.offset.set(slice_to_array_buffer_view(global.get_cx(), &result.parameters.borrow().offset));
+            let _ = TypedArray::<Float32>::create(global.get_cx(), result.parameters.borrow().offset.len() as u32,
+                Some(&result.parameters.borrow().offset), result.offset.handle_mut());


Let format this like:

let _ = Float32Array::create(global.get_cx(), result.parameters.borrow().offset.len() as u32, Some(&result.parameters.borrow().offset), result.offset.handle_mut());

jdm · 2017-02-09T19:17:08Z

components/script/dom/vrframedata.rs

+            let _ = TypedArray::<Float32>::create(global.get_cx(), matrix.len() as u32, Some(&matrix),
+                framedata.right_proj.handle_mut());
+            let _ = TypedArray::<Float32>::create(global.get_cx(), matrix.len() as u32, Some(&matrix),
+                framedata.right_view.handle_mut());


nit: use Float32Array::create and indent the second line to match global.get_cx().

jdm · 2017-02-09T19:17:08Z

components/script/dom/vrpose.rs

@@ -33,9 +33,13 @@ unsafe fn update_or_create_typed_array(cx: *mut JSContext,
    match src {
        Some(ref data) => {
            if dst.get().is_null() {
-                dst.set(slice_to_array_buffer_view(cx, &data));
+                let _ = TypedArray::<Float32>::create(cx, data.len() as u32, src,
+                    dst.handle_mut());


nit: Float32Array::create, and move dst.handle_mut() into the previous line.

jdm · 2017-02-09T19:17:08Z

components/script/dom/vrstageparameters.rs

+            let _ = TypedArray::<Float32>::create(global.get_cx(),
+                stage.parameters.borrow().sitting_to_standing_transform.len() as u32,
+                Some(&stage.parameters.borrow().sitting_to_standing_transform),
+                stage.transform.handle_mut());


nit: Float32Array::create, and indent each line to match global.get_cx().

yysung1123 · 2017-02-10T17:12:08Z

Fixed changes requested.

jdm

Sorry, one last issue that I noticed.

jdm · 2017-02-10T17:44:19Z

components/script/dom/vrframedata.rs

+            let _ = Float32Array::create(global.get_cx(), matrix.len() as u32, Some(&matrix),
+                                         framedata.right_proj.handle_mut());
+            let _ = Float32Array::create(global.get_cx(), matrix.len() as u32, Some(&matrix),
+                                         framedata.right_view.handle_mut());


This is a GC hazard right now - the GC does not know about framedata yet, so if any of the Float32Array::create calls trigger a GC, the existing typed arrays will be GCed and framedata will store invalid pointers. We should call reflect_dom_object first before creating the arrays.


        Remove slice_to_array_buffer_view and update_array_buffer_view

jdm · 2017-02-11T16:04:12Z

@bors-servo: r+

bors-servo · 2017-02-11T16:04:13Z

📌 Commit 2d99f7f has been approved by jdm

bors-servo · 2017-02-11T16:04:17Z

⌛ Testing commit 2d99f7f with merge 1ab3c34...


        Auto merge of #15427 - deror1869107:rewrite-webvr, r=jdm

Remove slice_to_array_buffer_view and update_array_buffer_view  Remove slice_to_array_buffer_view and update_array_buffer_view ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #15350 - [X] These changes do not require tests   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15427)

bors-servo · 2017-02-11T16:57:50Z

☀️ 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 1ab3c34 to master...

highfive assigned wafflespeanut Feb 7, 2017

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

jdm reviewed Feb 7, 2017

View changes

yysung1123 force-pushed the yysung1123:rewrite-webvr branch 2 times, most recently from 3085939 to d58082c Feb 7, 2017

jdm assigned jdm and unassigned wafflespeanut Feb 9, 2017

jdm requested changes Feb 9, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 9, 2017

yysung1123 force-pushed the yysung1123:rewrite-webvr branch from d58082c to 3ca2332 Feb 10, 2017

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

jdm requested changes Feb 10, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels Feb 10, 2017

jdm mentioned this pull request Feb 11, 2017

TypedArray::create API accept length or slice servo/rust-mozjs#337

Merged

yysung1123 force-pushed the yysung1123:rewrite-webvr branch from 3ca2332 to c670d3b Feb 11, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Feb 11, 2017

Remove slice_to_array_buffer_view and update_array_buffer_view

Loading status checks…

2d99f7f

yysung1123 force-pushed the yysung1123:rewrite-webvr branch from c670d3b to 2d99f7f Feb 11, 2017

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

bors-servo merged commit 2d99f7f into servo:master Feb 11, 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 11, 2017

servo / servo

Remove slice_to_array_buffer_view and update_array_buffer_view #15427

Remove slice_to_array_buffer_view and update_array_buffer_view #15427

yysung1123 commented Feb 7, 2017 •

edited by larsbergstrom

highfive commented Feb 7, 2017

highfive commented Feb 7, 2017

This comment has been minimized.

This comment has been minimized.

yysung1123 commented Feb 8, 2017

jdm left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

yysung1123 commented Feb 10, 2017 •

edited

jdm left a comment

This comment has been minimized.

jdm commented Feb 11, 2017

bors-servo commented Feb 11, 2017

bors-servo commented Feb 11, 2017

bors-servo commented Feb 11, 2017

servo / servo

Join GitHub today

Remove slice_to_array_buffer_view and update_array_buffer_view #15427

Remove slice_to_array_buffer_view and update_array_buffer_view #15427

Conversation

yysung1123 commented Feb 7, 2017 • edited by larsbergstrom

highfive commented Feb 7, 2017

highfive commented Feb 7, 2017

This comment has been minimized.

jdm Feb 7, 2017

This comment has been minimized.

yysung1123 Feb 7, 2017

yysung1123 commented Feb 8, 2017

jdm left a comment

This comment has been minimized.

jdm Feb 9, 2017

This comment has been minimized.

jdm Feb 9, 2017

This comment has been minimized.

jdm Feb 9, 2017

This comment has been minimized.

jdm Feb 9, 2017

yysung1123 commented Feb 10, 2017 • edited

jdm left a comment

This comment has been minimized.

jdm Feb 10, 2017

jdm commented Feb 11, 2017

bors-servo commented Feb 11, 2017

bors-servo commented Feb 11, 2017

bors-servo commented Feb 11, 2017

yysung1123 commented Feb 7, 2017 •

edited by larsbergstrom

yysung1123 commented Feb 10, 2017 •

edited