Skip to content
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

Implement DOMMatrix. #8509

Closed
Ms2ger opened this issue Nov 13, 2015 · 20 comments
Closed

Implement DOMMatrix. #8509

Ms2ger opened this issue Nov 13, 2015 · 20 comments

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Nov 13, 2015

Blocks #6097, #8822.

  • Spec: https://drafts.fxtf.org/geometry/#DOMMatrix
  • Code would be in new files (components/script/dom/dommatrix.rs, components/script/dom/webidls/DOMMatrix.webidl) and a mod dommatrix; in components/script/dom/mod.rs.
  • Test: ./tests/wpt/css-tests/geometry-1_dev/html/DOMMatrix-001.htm (metadata: ./tests/wpt/metadata-css/geometry-1_dev/html/DOMMatrix-001.htm.ini; see tests/wpt/README.md for more information).
@ajnirp
Copy link
Contributor

@ajnirp ajnirp commented Nov 14, 2015

can i work on this?

@jdm
Copy link
Member

@jdm jdm commented Nov 14, 2015

Yep!

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Jan 15, 2016

Still working on this @wenderen?

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 1, 2016

@wenderen Let us know if you want to pick this up again. In the meantime, I'm unassigning this issue.

@KiChjang KiChjang removed the C-assigned label Feb 1, 2016
@peterjoel
Copy link
Contributor

@peterjoel peterjoel commented Feb 14, 2016

I would like to pick this one up please.

@frewsxcv
Copy link
Member

@frewsxcv frewsxcv commented Feb 14, 2016

Go for it! Let us know if you run into any issues

@frewsxcv frewsxcv added the C-assigned label Feb 14, 2016
@peterjoel
Copy link
Contributor

@peterjoel peterjoel commented Feb 14, 2016

@frewsxcv I'm sure I will!
For starters, are there existing matrix operations, used internally by servo, that can be bound to?

I'm looking at this PR for DOMQuads, and it looks like a good reference for the amount and type of work that needs to be done. Is that a fair assessment?

@jdm
Copy link
Member

@jdm jdm commented Feb 15, 2016

The euclid library has matrix implementations: http://doc.servo.org/servo/euclid/struct.Matrix2D.html and http://doc.servo.org/servo/euclid/struct.Matrix4.html . The DOMQuads PR looks like a valid reference.

@peterjoel
Copy link
Contributor

@peterjoel peterjoel commented Feb 21, 2016

@jdm Is it a problem that euclid matrices use f32, while the webidl generated traits use f64?

@jdm
Copy link
Member

@jdm jdm commented Feb 21, 2016

Probably! Maybe create a new one?

@peterjoel
Copy link
Contributor

@peterjoel peterjoel commented Feb 21, 2016

Perhaps it would be better to make generic versions of the euclid objects and then make the existing ones aliases for those?

@jdm
Copy link
Member

@jdm jdm commented Feb 21, 2016

Seems reasonable to me!

@peterjoel
Copy link
Contributor

@peterjoel peterjoel commented Feb 29, 2016

I get an error when implementing a method that uses a Float32Array or Float64Array: "Can't handle SpiderMonkey interface arguments yet". This is coming from here.

Is it something I can fix easily, or should I just skip these for now?

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Feb 29, 2016

We currently don't support any sort of ArrayBuffers with our codegen. Tracking issue is #5014. Skip those for now.

@peterjoel
Copy link
Contributor

@peterjoel peterjoel commented Mar 3, 2016

I have a query about the existing test:
https://github.com/servo/servo/blob/master/tests/wpt/css-tests/geometry-1_dev/html/DOMMatrix-001.htm#L138

The test is failing because I don't throw a TypeError here. But this looks to be correct according to the spec. A 6 element sequence as the argument should represent a 2D array, and there is no mention of throwing errors based on the values themselves:
https://drafts.fxtf.org/geometry/#dom-dommatrixreadonly-dommatrixreadonly-numbersequence

@Ms2ger
Copy link
Contributor Author

@Ms2ger Ms2ger commented Mar 4, 2016

It seems to test that you can't construct a DOMMatrixReadOnly at all, which seems wrong. Feel free to ignore those tests.

@jdm jdm added the C-has open PR label Mar 31, 2016
@stjepang
Copy link
Contributor

@stjepang stjepang commented Mar 31, 2016

Any updates on this issue? :)
In case it got forgotten or abandoned, I'd be willing to implement it.

@jdm
Copy link
Member

@jdm jdm commented Mar 31, 2016

Nope, @peterjoel is actively working on it. There are just a bunch of moving pieces in other repositories which have ended up being necessary.

@jdm
Copy link
Member

@jdm jdm commented May 25, 2016

@peterjoel How's this progressing? Anything we can help with?

@peterjoel
Copy link
Contributor

@peterjoel peterjoel commented May 26, 2016

@jdm I've been busy with my job. But the feature is mostly done. Outstanding is just tests and the CSS-style initialisers. There is a 3-day weekend coming up this weekend, and I should get the PR in.

@peterjoel peterjoel mentioned this issue May 30, 2016
3 of 5 tasks complete
@peterjoel peterjoel mentioned this issue Jul 3, 2016
3 of 5 tasks complete
bors-servo added a commit that referenced this issue Jul 4, 2016
DOMMatrix and DOMMatrixReadOnly

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #8509.

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

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)
<!-- Reviewable:end -->
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 4, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 5, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 5, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 6, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 6, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 6, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 6, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 7, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 7, 2016
peterjoel added a commit to peterjoel/servo that referenced this issue Jul 7, 2016
bors-servo added a commit that referenced this issue Aug 29, 2016
DOMMatrix and DOMMatrixReadOnly

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #8509.

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

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 14, 2016
DOMMatrix and DOMMatrixReadOnly

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #8509.

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

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 14, 2016
DOMMatrix and DOMMatrixReadOnly

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #8509.

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

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Sep 16, 2016
DOMMatrix and DOMMatrixReadOnly

<!-- Please describe your changes on the following line: -->

---
<!-- 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 #8509.

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

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

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12202)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants
You can’t perform that action at this time.