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 value for DOMTokenList #9763

Merged
merged 1 commit into from Mar 1, 2016
Merged

Implement value for DOMTokenList #9763

merged 1 commit into from Mar 1, 2016

Conversation

@Tangresh
Copy link

Tangresh commented Feb 26, 2016

Fixes #9725

Review on Reviewable

@highfive
Copy link

highfive commented Feb 26, 2016

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

@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 26, 2016

You'll need to add spec links for those new methods to make ./mach test-tidy happy.

I'm also somewhat concerned that there's no more passing tests.

r? @nox

@highfive highfive assigned nox and unassigned glennw Feb 26, 2016
@Tangresh Tangresh force-pushed the Tangresh:i9725 branch from 2e6e1bb to 50b1017 Feb 26, 2016
@Tangresh
Copy link
Author

Tangresh commented Feb 26, 2016

I think there are 4 more tests that pass with this change, should there be more?

@Ms2ger
Copy link
Contributor

Ms2ger commented Feb 26, 2016

Yeah, but the interfaces test only checks that the attribute is implemented, not if it works correctly. I'll see if I can find anything better.

@nox
Copy link
Member

nox commented Feb 26, 2016

The new passing tests check that PutForwards work correctly, but none of them check the actual value of list.value. Trying the PR to see if there are more of them, but unfortunately I don't think there are any.

@bors-servo try

@bors-servo
Copy link
Contributor

bors-servo commented Feb 26, 2016

Trying commit 50b1017 with merge c31cb07...

bors-servo added a commit that referenced this pull request Feb 26, 2016
Implement value for DOMTokenList

Fixes #9725

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

bors-servo commented Feb 26, 2016

@nox
Copy link
Member

nox commented Feb 26, 2016

Could you add a test in tests/wpt/web-platform-tests/dom/nodes/Element-classlist.html that actually use the value attribute?

@Tangresh
Copy link
Author

Tangresh commented Feb 26, 2016

Is the Element-classlist test the right place for such a test? I found a test for DOMTokenList-stringifier, would an equivalent test for value be more appropriate?

@nox
Copy link
Member

nox commented Feb 26, 2016

@Tangresh Now that you mention it, it should just probably go in a new test.

./mach create-wpt tests/wpt/web-platform-tests/dom/lists/DOMTokenList-value.html
@nox nox removed the S-awaiting-review label Feb 27, 2016
@Tangresh Tangresh force-pushed the Tangresh:i9725 branch 2 times, most recently from 09a9a9e to d9a210a Feb 28, 2016
@nox
Copy link
Member

nox commented Feb 29, 2016

r=me once you squash the commits.

-S-awaiting-review -C-needs-test -S-needs-tests +S-needs-squash


Reviewed 5 of 5 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

Dominik Menzi
@Tangresh Tangresh force-pushed the Tangresh:i9725 branch from d9a210a to b29b2d6 Feb 29, 2016
@nox nox removed the S-needs-squash label Mar 1, 2016
@nox
Copy link
Member

nox commented Mar 1, 2016

@bors-servo
Copy link
Contributor

bors-servo commented Mar 1, 2016

📌 Commit b29b2d6 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Mar 1, 2016

Testing commit b29b2d6 with merge 0062870...

bors-servo added a commit that referenced this pull request Mar 1, 2016
Implement value for DOMTokenList

Fixes #9725

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

bors-servo commented Mar 1, 2016

@bors-servo bors-servo merged commit b29b2d6 into servo:master Mar 1, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Tangresh Tangresh deleted the Tangresh:i9725 branch Mar 1, 2016
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.