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

return null from GetNamespaceURI, not the empty string #3145

Merged
merged 1 commit into from Aug 27, 2014

Conversation

@ajnirp
Copy link
Contributor

ajnirp commented Aug 25, 2014

Fixes #3140

I tried changing the ini file for the Document-createElementNS.html test. I expected that changing the last three tests (the null / undefined / empty string namespace ones) to PASS would work, but apparently the tagName assert makes them fail. I guess that's a separate issue, so I left the ini file as it was.

@hoppipolla-critic-bot
Copy link

hoppipolla-critic-bot commented Aug 25, 2014

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

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.

@highfive
Copy link

highfive commented Aug 25, 2014

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

@Manishearth
Copy link
Member

Manishearth commented Aug 25, 2014

r+, waiting for Travis.

In the meantime, it might be interesting to try and find out why the tagName test failed and solve that too :)

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 25, 2014

@wenderen, I pointed out some now-passing tests on critic, and picked a few small nits while I was there. Thanks for your PR!

@Ms2ger
Copy link
Contributor

Ms2ger commented Aug 26, 2014

@wenderen, can you please squash the four commits and force-push, so I can merge? Thanks!

@ajnirp
Copy link
Contributor Author

ajnirp commented Aug 27, 2014

Dumb question: I just need to do git rebase -i HEAD~4, pick the most recent and squash the rest, right?

@Manishearth
Copy link
Member

Manishearth commented Aug 27, 2014

Yep. f (fixup) instead of squash is better, otherwise it clutters up the commit message

correctly mark tests
@ajnirp ajnirp force-pushed the ajnirp:NamespaceURI.None branch from dcb3ff7 to 32b7f5e Aug 27, 2014
Manishearth added a commit that referenced this pull request Aug 27, 2014
return null from GetNamespaceURI, not the empty string; r=Manishearth, Ms2ger
@Manishearth Manishearth merged commit aa52b0a into servo:master Aug 27, 2014
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
@ajnirp ajnirp deleted the ajnirp:NamespaceURI.None branch Aug 27, 2014
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.

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