Skip to content

/ html5ever

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

Properly encode the states for CDATA #224

Merged
merged 1 commit into from Oct 23, 2016
Merged

Properly encode the states for CDATA #224

merged 1 commit into from Oct 23, 2016

Conversation

@nox
Copy link
Member

nox commented Oct 23, 2016 •
edited by larsbergstrom

This change is Reviewable
@SimonSapin
Copy link
Member

SimonSapin commented Oct 23, 2016

tb: tests21.dat-11 (scripting disabled) is failing on Travis.

Did the spec change? Should html5lib-tests be updated accordingly?

Is this a breaking change because the State enum is public? Does it need to be? More generally, could we make the public API smaller so that we can change internals without breaking it?
@nox
Copy link
Member Author

nox commented Oct 23, 2016

It needs to be because the tree builder can change the state of the tokenizer. To any state. That should be improved but this is still a breaking change.

Will look into the failing test, that wasn't supposed to happen and I didn't even see it locally...
@nox
Properly encode the states for CDATA
Loading status checks…
e058a77
@nox nox force-pushed the nox:cdata branch from 89ebbdc to e058a77 Oct 23, 2016
@nox
Copy link
Member Author

nox commented Oct 23, 2016

@SimonSapin I fixed the bug.
@SimonSapin
Copy link
Member

SimonSapin commented Oct 23, 2016

IRC:

  • nox As for whether the spec changed, no idea, but I would rather we follow the states closely, and in the spec the CDATA states don't use look-ahead, so I think we should align ourselves.
  • SimonSapin: so it shouldn’t change the overall behavior of tokenizer + tree builder ?
  • nox: Yep.

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

bors-servo commented Oct 23, 2016

📌 Commit e058a77 has been approved by SimonSapin
@bors-servo
Copy link
Contributor

bors-servo commented Oct 23, 2016

⚡ Test exempted - status
@bors-servo bors-servo merged commit e058a77 into servo:master Oct 23, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
bors-servo added a commit that referenced this pull request Oct 23, 2016
@bors-servo
Auto merge of #224 - nox:cdata, r=SimonSapin
Loading status checks…
78c6995 
Properly encode the states for CDATA

<!-- Reviewable:start -->
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/224)
<!-- Reviewable:end -->
@nox nox deleted the nox:cdata branch Oct 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
@nox @SimonSapin @bors-servo
You can’t perform that action at this time.