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

Implement streaming support for TextDecoder #13234

Closed
Ms2ger opened this issue Sep 12, 2016 · 8 comments
Closed

Implement streaming support for TextDecoder #13234

Ms2ger opened this issue Sep 12, 2016 · 8 comments

Comments

@Ms2ger
Copy link
Contributor

@Ms2ger Ms2ger commented Sep 12, 2016

Blocks #5601.

@yodalee
Copy link
Contributor

@yodalee yodalee commented Oct 3, 2016

I wanna make sure that my thought is correct. To implement the streaming support, I think we need to add a RawDecoder, which is provided by encoding, to the TextDecoder
Currently, the decode method will process the input buffer to the end by calling encoding.decode(), thus we are unable to do incremental decoding. To allow stream decode, we need to add a RawDecoder and use raw_feed method instead.

@jdm
Copy link
Member

@jdm jdm commented Oct 3, 2016

That looks right to me.

@yodalee
Copy link
Contributor

@yodalee yodalee commented Oct 10, 2016

I cannot get it to work in the end. Here is the branch I tried so far.
master...yodalee:add-streaming-support-for-textdecoder
I add the stream options, Box<RawDecoder>, do not flush flag and BOMseen flags.
However I cannot get this work, since Decode method take &self instead of &mut self as arguments. So I cannot modify any flag or RawDecoder in TextDecoder since it is not mutable.
No idea how to implement this.

@jdm
Copy link
Member

@jdm jdm commented Oct 10, 2016

@yodalee You need to store mutable fields inside Cell or DOMRefCell in order to mutate them. Does that help?

@yodalee
Copy link
Contributor

@yodalee yodalee commented Nov 16, 2016

I got an idea that: I can add a RawDecoder into TextDecoder to support streaming. To achieve that, I need to make customized change to Rust-Encoding, adding a function to reset the unprocessed buffer in RawDecoder. However I cannot get it worked.
I download rust-encoding and modify Cargo.toml in component/script, changing encoding setting to

{path="localpath", version="0.2"}

However, the compiler error when compiling dom/htmlformelement.rs. Here is part of error message:

   --> /home/yodalee/mozilla/servo/components/script/dom/htmlformelement.rs:384:42
    |
384 |                  .encoding_override(Some(self.pick_encoding()))
    |                                          ^^^^^^^^^^^^^^^^^^^^ expected trait `html5ever::encoding::Encoding`, found trait `encoding::Encoding`
    |
    = note: expected type `&html5ever::encoding::Encoding + Send + Sync`
    = note:    found type `&'static encoding::Encoding + Send + Sync + 'static`

which I cannot understand how to fix it.

@jdm
Copy link
Member

@jdm jdm commented Mar 24, 2018

This requires adding the TextDecodeOptions dictionary from the spec and implementing the steps of the encode method that relate to streaming.

@talklittle
Copy link
Contributor

@talklittle talklittle commented Mar 25, 2018

I'd like to give this a shot.

@jdm jdm added the C-assigned label Mar 25, 2018
bors-servo added a commit that referenced this issue Mar 26, 2018
TextDecoder: streaming decode, ignoreBOM

<!-- Please describe your changes on the following line: -->
Implement streaming decode and ignoreBOM flag for TextDecoder.

https://encoding.spec.whatwg.org/#dom-textdecoder-decode

https://encoding.spec.whatwg.org/#dom-textdecoder-ignorebom

---
<!-- 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 #13234 (github issue number if applicable).
- [x] These changes fix #5600 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because the wpt tests are used for testing:

* /encoding/textdecoder-fatal-streaming.html
* /encoding/textdecoder-streaming.html
* /encoding/textdecoder-ignorebom.html

<!-- 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/20431)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this issue Mar 26, 2018
TextDecoder: streaming decode, ignoreBOM

<!-- Please describe your changes on the following line: -->
Implement streaming decode and ignoreBOM flag for TextDecoder.

https://encoding.spec.whatwg.org/#dom-textdecoder-decode

https://encoding.spec.whatwg.org/#dom-textdecoder-ignorebom

---
<!-- 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 #13234 (github issue number if applicable).
- [x] These changes fix #5600 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because the wpt tests are used for testing:

* /encoding/textdecoder-fatal-streaming.html
* /encoding/textdecoder-streaming.html
* /encoding/textdecoder-ignorebom.html

<!-- 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/20431)
<!-- 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.

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