Use RangeError for TextEncoder/TextDecoder

[Ms2ger]

Blocks #5601.

• Spec: https://encoding.spec.whatwg.org/#dom-textdecoder, https://encoding.spec.whatwg.org/#dom-textencoder.
• Code: components/script/dom/textdecoder.rs, components/script/dom/textencoder.rs, components/script/dom/bindings/error.rs.
• Test: possibly /encoding/api-replacement-encodings.html, /encoding/textencoder-constructor-non-utf.html.

To fix this bug, you'll first need to implement RangeError exceptions. This should be analogous to the TypeError implementation that already exists in components/script/dom/bindings/error.rs. I'd extend get_error_message to accept one error number for TypeError and one for RangeError, and create a second static like ERROR_FORMAT_STRING that uses JSEXN_RANGEERR.

Then you'll need to return Err(Error::Range) in the places that currently have TODO/FIXME comments mentioning "RangeError".

Bonus point for writing additional tests for the case where the encoding argument is not an encoding at all.

[aneeshusa]

I'd like to give this a try.

[jdm]

Go for it!

[aneeshusa]

A few questions about the tests.

• Test naming: I had to change some of the test description strings in order to get the tests to pass. The test logic itself in the functions passed to test() is fine, but for some reason the test name seems to be affecting the test results.
  • Is changing the test names the correct way to fix this, or is there a better way to do it? The test logic looks fine as is, and changing the name was not intuitive for me and took me a long time to guess. The function passed to test() should encode all of the necessary information, so I don't see why the test name should be a factor.
  • Out of curiousity, why/how does this work? I think that having a 'not' present in the name is setting whether or not the test is expected to PASS or FAIL, but I'm not sure - I couldn't find the name field being accessed in any meaningful way in the test runner.
• GBK Encoding: This is a special case, and it's the only test that doesn't work after twiddling test names.
  • The spec says that the gbk decoder is the gb18030 decoder.
  • The encoding crate we use lumps them together and says that the gbk labels map to gb18030, which I think is reasonable.
  • Our tests say that each encoding should map to itself, which is also reasonable. This implies gbk should map to the gbk decoder, which happens to be the gbk18030 decoder under the hood.
  • Firefox reports a gbk decoder for gbk, as does Chromium.
  • Should I change the test case or should we consider this an upstream bug?

I'll look into writing the not-even-an-encoding test cases once I get the current set to pass.

[jdm]

@aneeshusa: Expected test result come from the ini files under tests/wpt/meta, like http://mxr.mozilla.org/servo/source/tests/wpt/metadata/encoding/api-replacement-encodings.html.ini . If you're seeing output like "PASS expected FAIL", that means that your changes are causing the checks to succeed, but the ini files currently record an expected failure instead. Does that clear it up?

[aneeshusa]

OK, thanks, that's a much better explanation than name-changing voodoo. I've gotten the rest of the tests to pass, what should I do about the gbk test (see my previous comment)?

[aneeshusa]

Also, is there an official way to only run certain wpt tests via mach? I've been using a hacky solution, but if there's a better way that would be much appreciated.

[Ms2ger]

./mach test-wpt --include ...; I'll need to investigate what's up with gbk

[jdm]

You can also just run ./mach test tests/wpt/web-platform-tests/subdir or even ./mach test tests/wpt/web-platform-tests/path/to/file.html.

[aneeshusa]

Also, as far as I can tell the rust-encoding crate is wrong about the whatwg name for utf-16le and utf-16, so I'll put in a PR upstream.

[aneeshusa]

Perfect, the --include option is what I was looking for. Thanks!

[aneeshusa]

Btw, my thoughts on gbk are that gbk should map to gbk instead of gbk18030, as per the spec, implying a fix would be needed upstream.

[Ms2ger]

Gbk looks like it needs an upstream fix, yeah.

[aneeshusa]

Just put in the PR for utf-16le. Working on one for gbk right now, and investigating a potential issue with hz-gb-2312.

[aneeshusa]

The utf-16le PR just got merged, and I also just put in a PR to fix GBK encoding.

[lifthrasiir]

cc me

[aneeshusa]

I just tried to bump the components/script crate's version dependency on the encoding crate to 0.2.30, which includes the utf-16le changes so I could test if they worked. Unfortunately, something is limiting the version number to 0.2.25, and I haven't been able to figure out which crate is the cause; the error message isn't very informative and grep isn't helping either. Does anyone know which crate is the impediment?

[aneeshusa]

Also, I opened an issue for a deviation from the spec for hz-gb-2312; if the current implementation is kept it isn't too hard to work around in servo (just an extra pattern in a match statement), so I'm not too worried about which way that goes.

[Ms2ger]

I would suggest just fixing the RangeError issue, and your upstream changes will be pulled in at some point later.