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

Use BufferSource in TextDecoder API #20344

Closed
jdm opened this issue Mar 19, 2018 · 11 comments
Closed

Use BufferSource in TextDecoder API #20344

jdm opened this issue Mar 19, 2018 · 11 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Mar 19, 2018

Now that #20267 has merged, we can uncomment the decode method in TextDecoder.webidl that accepts BufferSource and remove the one that fakes it by accepting an option value. This will require similar cleanup as #20342.

Code: components/script/dom/webidls/TextDecoder.webidl, components/script/dom/textdecoder.rs
Tests: ./mach test-wpt tests/wpt/web-platform-tests/encoding/textdecoder*

@christianpoveda
Copy link
Contributor

@christianpoveda christianpoveda commented Mar 23, 2018

@highfive assign me

@highfive highfive added the C-assigned label Mar 23, 2018
@highfive
Copy link

@highfive highfive commented Mar 23, 2018

Hey @christianpoveda! Thanks for your interest in working on this issue. It's now assigned to you!

@christianpoveda
Copy link
Contributor

@christianpoveda christianpoveda commented Mar 24, 2018

Just started working on it. I have a first working version here. Nevertheless a bunch of tests are failing. They are failing because fatal is set to true and in cases where the decoding is supposed to fail Decode does not throw anything. For example:

 FAIL [expected PASS] Fatal flag: utf-16le - truncated code unit
  │   → assert_throws: function "function () {
            new TextDecoder(t.encoding, {fatal: true}).decode(new Uint8Array(t.input))
        }" did not throw

However reading the spec, {fatal: true} should be passed as a second parameter to decode (which is the options parameter in the rust code) not to TextDecoder's constructor.

Should I modify the tests? or should I keep using self.fatal instead of options.fatal?

PD: when using self.fatal all the tests pass.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 24, 2018

We should update our implementation to match the specification: https://encoding.spec.whatwg.org/#interface-textdecoder

@christianpoveda
Copy link
Contributor

@christianpoveda christianpoveda commented Mar 24, 2018

Ohh sorry I was confused, I did not realized TextDecodeOptions exists and is not just a typo. Nevertheless what should I do with TextDecodeOptions and stream?

Also, the specification says Decode does not throw exceptions, but https://encoding.spec.whatwg.org/#dom-textdecoder-decode says it could throw an TypeError exception

@jdm
Copy link
Member Author

@jdm jdm commented Mar 24, 2018

Let's accept a TextDecodeOptions as the second argument but ignore the stream member for the purposes of this issue. We can file another issue to implement the steps in https://encoding.spec.whatwg.org/#dom-textdecoder-decode that it affects.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 24, 2018

Actually, we shouldn't need to add TextDecodeOptions for this issue at all. I'll file the issue for updating the implementation to match the spec.

@jdm
Copy link
Member Author

@jdm jdm commented Mar 24, 2018

Ah, that's #13234.

@christianpoveda
Copy link
Contributor

@christianpoveda christianpoveda commented Mar 24, 2018

hehe, Ok i'll let the TextDecodeOptions in the webidl file but will not use it in the body of Decode. What about the exceptions?

@jdm
Copy link
Member Author

@jdm jdm commented Mar 24, 2018

What do you mean by "the specification says Decode does not throw exceptions"? If you mean that the spec webIDL doesn't include [Throws], that's because that's an implementation-specific extension that we use.

@christianpoveda
Copy link
Contributor

@christianpoveda christianpoveda commented Mar 24, 2018

oh ok got it

bors-servo added a commit that referenced this issue Mar 24, 2018
TextDecoder's Decode now receives a BufferSource as input

<!-- Please describe your changes on the following line: -->

r? jdm

---
<!-- 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 #20344 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20413)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 24, 2018
TextDecoder's Decode now receives a BufferSource as input

<!-- Please describe your changes on the following line: -->

r? jdm

---
<!-- 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 #20344 (github issue number if applicable).

<!-- Either: -->
- [X] There are tests for these changes OR
- [ ] These changes do not require tests because _____

<!-- 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. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20413)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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