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 DOMPoint and DOMPointReadOnly #6683

Merged
merged 1 commit into from Jul 23, 2015
Merged

Conversation

@tschneidereit
Copy link
Contributor

tschneidereit commented Jul 21, 2015

Review on Reviewable

@highfive
Copy link

highfive commented Jul 21, 2015

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

@pcwalton
Copy link
Contributor

pcwalton commented Jul 21, 2015

r? @jdm

@Ms2ger Ms2ger self-assigned this Jul 21, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 22, 2015

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


Reviewed 6 of 6 files at r1, 40 of 40 files at r2.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.


components/script/dom/dompoint.rs, line 13 [r2] (raw file):
Spec pointer somewhere around here.


components/script/dom/dompoint.rs, line 50 [r2] (raw file):
-> () is typically only written out in generated code.


components/script/dom/dompointreadonly.rs, line 13 [r2] (raw file):
Spec link


components/script/dom/dompointreadonly.rs, line 58 [r2] (raw file):
Rm


components/script/dom/webidls/DOMPoint.webidl, line 13 [r2] (raw file):
No Pref.


components/script/dom/webidls/DOMPoint.webidl, line 14 [r2] (raw file):
This constructor doesn't exist in the spec for some irresponsible reason.


components/script/dom/webidls/DOMPointReadOnly.webidl, line 13 [r2] (raw file):
No Pref, and needs a spec link for the interface. Also needs a constructor.


Comments from the review on Reviewable.io

@tschneidereit
Copy link
Contributor Author

tschneidereit commented Jul 22, 2015

Thanks for the review, I think this addresses all comments. As discussed on IRC, I left out the DOMPoint(ReadOnly).fromPoint implementations because they're not implemented anywhere yet and might go away. Removing the DOMPointInit-using constructor and making DOMPointReadOnly constructible made more than half of the tests fail, but given that that's what the spec says, that's probably the tests' fault.
r? @Ms2ger


Review status: 39 of 46 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


components/script/dom/dompoint.rs, line 50 [r2] (raw file):
Right, I just copied the definitions from the generated bindings, but should've cleaned them up.


Comments from the review on Reviewable.io

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 22, 2015

-S-awaiting-review +S-needs-squash

Squash all four commits and ping me, please.


Reviewed 4 of 4 files at r3, 3 of 3 files at r4.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@tschneidereit tschneidereit force-pushed the tschneidereit:dompoint branch from b88938f to 1790008 Jul 22, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 22, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jul 22, 2015

📌 Commit 1790008 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2015

Testing commit 1790008 with merge 449fe56...

bors-servo pushed a commit that referenced this pull request Jul 23, 2015
Implement DOMPoint and DOMPointReadOnly



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6683)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2015

💔 Test failed - mac1

@jdm
Copy link
Member

jdm commented Jul 23, 2015


/_mozilla/mozilla/interfaces.html
---------------------------------
FAIL Interfaces exposed on the window
@jdm jdm added S-tests-failed and removed S-awaiting-merge labels Jul 23, 2015
Passes some but not all WPT tests. One failure is caused by an issue in codegen for the `DOMPointInit` dictionary, the others by outdated tests: Gecko implements an old version of the spec that overloaded the `DOMPoint` constructor to optionally take an object as the first argument, and made `DOMPointReadOnly` non-constructible.
@tschneidereit tschneidereit force-pushed the tschneidereit:dompoint branch from 1790008 to b692187 Jul 23, 2015
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 23, 2015

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2015

📌 Commit b692187 has been approved by Ms2ger

@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2015

Testing commit b692187 with merge 658c3d0...

bors-servo pushed a commit that referenced this pull request Jul 23, 2015
Implement DOMPoint and DOMPointReadOnly



<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/6683)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Jul 23, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3

@bors-servo bors-servo merged commit b692187 into servo:master Jul 23, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

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