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

Join tokens when stringifying DOMTokenList #6555

Merged
merged 4 commits into from Jul 7, 2015
Merged

Join tokens when stringifying DOMTokenList #6555

merged 4 commits into from Jul 7, 2015
+35 −34

Conversation

@frewsxcv
Copy link
Member

frewsxcv commented Jul 5, 2015

Review on Reviewable
@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Jul 5, 2015

Critic review: https://critic.hoppipolla.co.uk/r/5467

This is an external review system which you may optionally use for the code review of your pull request.

In order to help critic track your changes, please do not make in-place history rewrites (e.g. via git rebase -i or git commit --amend) when updating this pull request.
@frewsxcv frewsxcv force-pushed the frewsxcv:stringify-tokenlist branch from 160c61e to ff2a3b9 Jul 5, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 5, 2015

See individual commit descriptions for more information
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 5, 2015

Related to #1717
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 5, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 5, 2015

📌 Commit ff2a3b9 has been approved by Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Jul 5, 2015

⌛ Testing commit ff2a3b9 with merge 4bcdb22...
bors-servo pushed a commit that referenced this pull request Jul 5, 2015
bors-servo
Auto merge of #6555 - frewsxcv:stringify-tokenlist, r=Ms2ger
4bcdb22 
Join tokens when stringifying DOMTokenList



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

bors-servo commented Jul 5, 2015

💔 Test failed - mac1
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 5, 2015

Both commits have the same issue: they return strings with a leading space.
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 5, 2015

Ah, you're right, I'll fix it
@frewsxcv frewsxcv force-pushed the frewsxcv:stringify-tokenlist branch from ff2a3b9 to 6b9c1d9 Jul 5, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 5, 2015

Fixed in the latest force push
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 5, 2015

Can we share the code too?
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 5, 2015

Can we share the code too?

Is there a better place to put it than util::str as a utility function join_atoms? It'd create a dependency in the util create on string_cache.
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 6, 2015

Mm, that sucks. A better solution would be to make Atom implement AsRef<str> and then use that in util. I can land it like this if you push back, though :)
@Ms2ger Ms2ger self-assigned this Jul 6, 2015
@Ms2ger Ms2ger closed this Jul 6, 2015
@Ms2ger Ms2ger reopened this Jul 6, 2015
frewsxcv added a commit to frewsxcv/string-cache that referenced this pull request Jul 6, 2015
@frewsxcv
Implement AsRef<str> for Atom
09876f7 
servo/servo#6555 (comment)
@frewsxcv frewsxcv mentioned this pull request Jul 6, 2015
Merged
bors-servo pushed a commit to servo/string-cache that referenced this pull request Jul 6, 2015
bors-servo
Auto merge of #90 - frewsxcv:atom-as-ref-str, r=Ms2ger
Loading status checks…
6428d32 
Implement AsRef<str> for Atom

servo/servo#6555 (comment)
@frewsxcv
Join Atoms without requiring intermediate Vec
9b2ba3d 
Related to: servo/string-cache#89
frewsxcv added 3 commits Jul 5, 2015
@frewsxcv
Join tokens when stringifying DOMTokenList
91b0671 
Previous, it would return the original String straight from the
AttrValue, which might contain extraaneous whitespace. The spec
specifies to just join the tokens together with \x20

https://dom.spec.whatwg.org/#stringification-behavior
@frewsxcv
Update string_cache: 0.1.4 -> 0.1.5
c77b5aa
@frewsxcv
Create and utilize utility for joining strs
Loading status checks…
7159fe7
@frewsxcv frewsxcv force-pushed the frewsxcv:stringify-tokenlist branch from 6b9c1d9 to 7159fe7 Jul 7, 2015
@frewsxcv
Copy link
Member Author

frewsxcv commented Jul 7, 2015

Force pushed with the changes requested
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 7, 2015

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Jul 7, 2015

📌 Commit 7159fe7 has been approved by Ms2ger
@bors-servo
Copy link
Contributor

bors-servo commented Jul 7, 2015

⌛ Testing commit 7159fe7 with merge d64f918...
bors-servo pushed a commit that referenced this pull request Jul 7, 2015
bors-servo
Auto merge of #6555 - frewsxcv:stringify-tokenlist, r=Ms2ger
Loading status checks…
d64f918 
Join tokens when stringifying DOMTokenList



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

bors-servo commented Jul 7, 2015

☀️ Test successful - android, gonk, linux1, linux2, linux3, mac1, mac2, mac3
@bors-servo bors-servo merged commit 7159fe7 into servo:master Jul 7, 2015
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@frewsxcv frewsxcv deleted the frewsxcv:stringify-tokenlist branch Oct 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Ms2ger Ms2ger

Labels
S-awaiting-merge
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
@frewsxcv @hoppipolla-critic-bot @Ms2ger @bors-servo @jdm @highfive
You can’t perform that action at this time.