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

Encoding: encodeInto() #14505

Merged
merged 8 commits into from
Jan 9, 2019
Merged

Encoding: encodeInto() #14505

merged 8 commits into from
Jan 9, 2019

Conversation

annevk
Copy link
Member

@annevk annevk commented Dec 13, 2018

@annevk
Copy link
Member Author

annevk commented Dec 13, 2018

I found it a little hard to write tests without an implementation, but I think this framework is adequate for any number of tests we'd want to write. Probably at this point it's most helpful to review that framework and then when implementations start we can fill out the number of tests?

@ricea
Copy link
Contributor

ricea commented Dec 13, 2018

Looks good. I think this is a good framework to test encodeInto(). I haven't actually checked the byte values, I'm assuming they're correct.

Edit: I think we will need a separate test that bytes after the last written codepoint are not modified. With this test framework we can't tell whether they're left alone or overwritten with null bytes.

Copy link
Member

@hsivonen hsivonen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks.

To test an edge case that's hard for encoding_rs, it would be good to add a test whose output buffer size is 4 and whose input is an ASCII character followed by a snowman (or other 3-bytes-as-UTF-8 character).

Also: Output buffer size 4 and input contains two 2-bytes-as-UTF-8 characters.

@inexorabletash
Copy link
Member

Mentioned on the spec PR but:

Needs cases where the destination view is offset/subset of the backing array buffer, e.g.

const ab = new ArrayBuffer(5);
const view = new Uint8Array(ab, 1, 2);
new TextEncoder().encodeInto("ABCDE", view);

@annevk
Copy link
Member Author

annevk commented Dec 13, 2018

I wrote tests for more complicated view setups which was a lot of fun, but an implementation would be good at this point as I'm worried for off-by-one errors.

(I also added Henri's tests. I think I covered everything now.)

@annevk
Copy link
Member Author

annevk commented Dec 14, 2018

I've written all the various tests requested thus far, I think, including some additional ones I thought of myself.

@foolip
Copy link
Member

foolip commented Dec 19, 2018

FYI, I've rebased this branch to trigger "Travis CI - Pull Request", manually setting things right after the travis-ci.com transition in #14499 changed the name of the required check, which applies retroactively.

@annevk
Copy link
Member Author

annevk commented Dec 21, 2018

Does someone want to review the additions?

@hsivonen
Copy link
Member

hsivonen commented Jan 8, 2019

@foolip, the chrome[experimental] and firefox[experimental] tasks started 20 days ago but don't show up as finished.

@foolip
Copy link
Member

foolip commented Jan 8, 2019

@lukebjerring can you take a look at what's going on with the wpt.fyi checks? I guess this is web-platform-tests/wpt.fyi#950.

Copy link
Contributor

@ricea ricea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a slow and dumb polyfill. With the change I noted the tests pass.

encoding/encodeInto.any.js Outdated Show resolved Hide resolved
@lukebjerring
Copy link
Contributor

Yep, attempting to update says:

Could not find runs to compare: no test run found for firefox @ 60bfa1e with label pr_base

@annevk
Copy link
Member Author

annevk commented Jan 8, 2019

I plan to land these tomorrow, together with the change to the Encoding Standard.

Thanks for the review @ricea!

@lukebjerring
Copy link
Contributor

Looks like @Hexcles has succeeded in uploading empty reports 🎉
Now we can easily see the outcome is CRASH in Chrome and TIMEOUT in Safari.
https://staging.wpt.fyi/results/?pr=14505

Please fix that before this lands.

@annevk
Copy link
Member Author

annevk commented Jan 8, 2019

@lukebjerring can you reproduce it? I can't.

@annevk annevk merged commit 9ad263d into master Jan 9, 2019
@annevk annevk deleted the annevk/encodeInto branch January 9, 2019 07:52
annevk added a commit to whatwg/encoding that referenced this pull request Jan 9, 2019
This enables converting strings into UTF-8 byte sequences reusing a pre-allocated output buffer.

Also cleans up TextEncoder a bit.

Tests: web-platform-tests/wpt#14505.

Fixes #69.
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 3, 2019
… r=hsivonen.

web-platform-tests/wpt#14505
web-platform-tests/wpt#14775

UltraBlame original commit: 3c73631c8237077acb3795e51a2a2ea6016247df
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 3, 2019
… r=hsivonen.

web-platform-tests/wpt#14505
web-platform-tests/wpt#14775

UltraBlame original commit: 3c73631c8237077acb3795e51a2a2ea6016247df
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 3, 2019
… r=hsivonen.

web-platform-tests/wpt#14505
web-platform-tests/wpt#14775

UltraBlame original commit: 3c73631c8237077acb3795e51a2a2ea6016247df
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants