Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upChanged error type on qname, per spec #25223
Conversation
highfive
commented
Dec 10, 2019
|
Heads up! This PR modifies the following files:
|
|
@bors-servo r+ |
|
|
| Err(Error::Namespace) | ||
| }, | ||
| XMLName::InvalidXMLName => Err(Error::InvalidCharacter), | ||
| XMLName::Name => Err(Error::InvalidCharacter), |
This comment has been minimized.
This comment has been minimized.
jdm
Dec 10, 2019
Member
My reading of https://dom.spec.whatwg.org/#validate says that XMLName shouldn't throw anything. How do the test results look if that's the case?
This comment has been minimized.
This comment has been minimized.
Manishearth
Dec 10, 2019
Member
Oh, I misread this as being "does not match Name", but that's not the case
This comment has been minimized.
This comment has been minimized.
pshaughn
Dec 10, 2019
Author
Member
Good point, maybe there's more than just the exception-type to fix here. I'll find out.
This comment has been minimized.
This comment has been minimized.
pshaughn
Dec 10, 2019
Author
Member
When I change it to let Name in instead of throwing InvalidCharacter for it, then some tests go from correctly throwing InvalidCharacter to incorrectly not throwing, like Document.implementation.createDocumentType("prefix::local", "", "") is expected to throw Invalid Character. "prefix::local" appears to match the Name production. The test seems more consistent with just "if qualifiedName does not match the QName production."
This comment has been minimized.
This comment has been minimized.
pshaughn
Dec 10, 2019
Author
Member
However I should check the codepaths for those "" parts too, maybe those are what's supposed to cause the error this time.
This comment has been minimized.
This comment has been minimized.
pshaughn
Dec 10, 2019
Author
Member
https://dom.spec.whatwg.org/#interface-documenttype nope those are totally allowed to be empty strings, the error has to be for the prefix::local part
|
@bors-servo r- |
|
|
|
Could I get someone's opinion on what the specs say to do vs. what the tests say to do vs. what the code does? I'm not sure which part's out of sync. |
|
Sorry about the delay; been doing various travel things. I would file an issue on the spec and tag whoever has touched that area recently. Typically I've found that the tests for this kind of thing are more "correct" (in that they match what other browsers do). You should also check what other browsers do, and if it doesn't match the spec then get the spec changed. |
|
It turns out there's already an issue: whatwg/dom#671 |
|
(this push is just a rebase, not a functional update) |
|
Based on that issue thread I think it's safe to assume the spec change reflects reality and will eventually land; I'm going to rebase again and add a comment about the issue. |
|
This is old enough some test might have slid out from under it... |
Changed error type on qname, per spec <!-- Please describe your changes on the following line: --> As specified in https://dom.spec.whatwg.org/#validate invalid QNames when creating a namespaced document/element/attribute get an invalid character exception, not a namespace exception. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25110 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
|
@bors-servo try- r+ |
|
|
Changed error type on qname, per spec <!-- Please describe your changes on the following line: --> As specified in https://dom.spec.whatwg.org/#validate invalid QNames when creating a namespaced document/element/attribute get an invalid character exception, not a namespace exception. --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #25110 <!-- Either: --> - [X] There are tests for these changes <!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.--> <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
|
|
pshaughn commentedDec 10, 2019
As specified in https://dom.spec.whatwg.org/#validate invalid QNames when creating a namespaced document/element/attribute get an invalid character exception, not a namespace exception.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors