Skip to content

/ servo

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle detached array in XRView #26185

Merged
merged 1 commit into from Apr 15, 2020
Merged

handle detached array in XRView #26185

merged 1 commit into from Apr 15, 2020

Conversation

@daemon1024
Copy link
Contributor

daemon1024 commented Apr 14, 2020 •
edited

Recompute XRView.projectionmatrix when the buffer is detached

  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #26153 (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because ___
@highfive
Copy link

highfive commented Apr 14, 2020

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @jdm (or someone else) soon.
@highfive
Copy link

highfive commented Apr 14, 2020

Heads up! This PR modifies the following files:
@highfive
Copy link

highfive commented Apr 14, 2020

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth Manishearth self-requested a review Apr 14, 2020
@Manishearth Manishearth assigned Manishearth and unassigned jdm Apr 14, 2020
@Manishearth
Manishearth reviewed Apr 14, 2020
View changes
components/script/dom/xrview.rs Outdated
@@ -24,17 +25,24 @@ pub struct XRView {
proj: Heap<*mut JSObject>,
#[ignore_malloc_size_of = "mozjs"]
view: Heap<*mut JSObject>,
viewsrc: Vec<f32>,

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Apr 14, 2020

Member

let's call this proj_array, here and elsewhere in the code

Suggested change
viewsrc: Vec<f32>,
proj_array: Vec<f32>,
components/script/dom/xrview.rs Outdated
global,
);
let projm = view.projection.to_row_major_array();
let xrv = Box::new(XRView::new_inherited(

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Apr 14, 2020

Member

We typically keep the reflect_dom_object in one statement
components/script/dom/xrview.rs Outdated
Box::new(XRView::new_inherited(session, &transform, eye)),
global,
);
let projm = view.projection.to_row_major_array();

This comment has been minimized.

Sign in to view
Copy link
@Manishearth

Manishearth Apr 14, 2020

Member

this is already done below, you should just shift this line down
@daemon1024 daemon1024 requested a review from Manishearth Apr 14, 2020
@Manishearth
Manishearth approved these changes Apr 14, 2020
View changes
Copy link
Member

Manishearth left a comment

Please squash your commits and we can land this!
@daemon1024
handle detached array in XRView
Loading status checks…
a79f97b
@daemon1024 daemon1024 force-pushed the daemon1024:handle-null-proj branch from 76e1f6a to a79f97b Apr 15, 2020
@daemon1024 daemon1024 requested a review from Manishearth Apr 15, 2020
@Manishearth
Copy link
Member

Manishearth commented Apr 15, 2020

@bors-servo r+

thanks!
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

📌 Commit a79f97b has been approved by Manishearth
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

⌛ Testing commit a79f97b with merge f4d9b62...
bors-servo added a commit that referenced this pull request Apr 15, 2020
@bors-servo
Auto merge of #26185 - daemon1024:handle-null-proj, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
f4d9b62 
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Apr 15, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

⌛ Testing commit a79f97b with merge 583a721...
bors-servo added a commit that referenced this pull request Apr 15, 2020
@bors-servo
Auto merge of #26185 - daemon1024:handle-null-proj, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
583a721 
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Apr 15, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

⌛ Testing commit a79f97b with merge 21d1585...
bors-servo added a commit that referenced this pull request Apr 15, 2020
@bors-servo
Auto merge of #26185 - daemon1024:handle-null-proj, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
21d1585 
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Apr 15, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

⌛ Testing commit a79f97b with merge 249742a...
bors-servo added a commit that referenced this pull request Apr 15, 2020
@bors-servo
Auto merge of #26185 - daemon1024:handle-null-proj, r=Manishearth
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
249742a 
handle detached array in XRView

<!-- Please describe your changes on the following line: -->
Recompute XRView.projectionmatrix when the buffer is detached
---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #26153 (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

💔 Test failed - status-taskcluster
@jdm
Copy link
Member

jdm commented Apr 15, 2020

@bors-servo retry
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

⌛ Testing commit a79f97b with merge 324e2d2...
@bors-servo
Copy link
Contributor

bors-servo commented Apr 15, 2020

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing 324e2d2 to master...
@bors-servo bors-servo merged commit 324e2d2 into servo:master Apr 15, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Manishearth Manishearth

Assignees

@Manishearth Manishearth

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

Handle detached arrays in XRView
5 participants
@daemon1024 @highfive @Manishearth @bors-servo @jdm
You can’t perform that action at this time.