Closed Bug 1257877 Opened 9 years ago Closed 9 years ago

Remove support for UTF-16 from TextEncoder

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: hsivonen, Assigned: emk)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, site-compat)

Attachments

(2 files, 1 obsolete file)

https://github.com/whatwg/encoding/commit/8360f775c8df145f649047c7d59c5ff733ade112 removed support for UTF-16 in TextEncoder. We should make our implementation reflect the spec change.
Attached patch patch (deleted) — Splinter Review
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8732513 - Flags: review?(hsivonen)
Attached patch wpt changes (obsolete) (deleted) — Splinter Review
Attachment #8732514 - Flags: review?(Ms2ger)
Comment on attachment 8732513 [details] [diff] [review] patch Review of attachment 8732513 [details] [diff] [review]: ----------------------------------------------------------------- Thank you! (I'm a bit uneasy about not throwing if there's an argument to the constructor that's not an ASCII case-insensitive match for the string string "utf-8", but that's really a spec concern, since the patch implements the spec.)
Attachment #8732513 - Flags: review?(hsivonen) → review+
Comment on attachment 8732514 [details] [diff] [review] wpt changes Review of attachment 8732514 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/web-platform/tests/encoding/api-basics.html @@ +14,5 @@ > assert_array_equals(new TextEncoder().encode(undefined), [], 'input default should be empty string') > }, 'Default inputs'); > > > function testEncodeDecodeSample(encoding, string, bytes) { Do we need this function if it has just one caller? ::: testing/web-platform/tests/encoding/textencoder-constructor-non-utf.html @@ +17,5 @@ > } else { > test(function() { > assert_equals(new TextDecoder(encoding.name).encoding, encoding.name); > + assert_equals(new TextEncoder(encoding.name).encoding, 'utf-8'); > + }, 'Non-UTF-8 encodings supported only for decode, not encode: ' + encoding.name); I'd prefer section.encodings.forEach(function(encoding) { if (encoding.name !== 'replacement') { test(function() { assert_equals(new TextDecoder(encoding.name).encoding, encoding.name); }, "Encoding argument supported for decode: ' + encoding.name); } test(function() { assert_equals(new TextEncoder(encoding.name).encoding, 'utf-8'); }, "Encoding argument not considered for encode: ' + encoding.name); });
Attached patch wpt changes, v2 (deleted) — Splinter Review
(In reply to :Ms2ger from comment #5) > ::: testing/web-platform/tests/encoding/api-basics.html > @@ +14,5 @@ > > assert_array_equals(new TextEncoder().encode(undefined), [], 'input default should be empty string') > > }, 'Default inputs'); > > > > > > function testEncodeDecodeSample(encoding, string, bytes) { > > Do we need this function if it has just one caller? Maybe not. Merged with the caller. > ::: testing/web-platform/tests/encoding/textencoder-constructor-non-utf.html > @@ +17,5 @@ > > } else { > > test(function() { > > assert_equals(new TextDecoder(encoding.name).encoding, encoding.name); > > + assert_equals(new TextEncoder(encoding.name).encoding, 'utf-8'); > > + }, 'Non-UTF-8 encodings supported only for decode, not encode: ' + encoding.name); > > I'd prefer > > section.encodings.forEach(function(encoding) { > if (encoding.name !== 'replacement') { > test(function() { > assert_equals(new TextDecoder(encoding.name).encoding, > encoding.name); > }, "Encoding argument supported for decode: ' + encoding.name); > } > > test(function() { > assert_equals(new TextEncoder(encoding.name).encoding, 'utf-8'); > }, "Encoding argument not considered for encode: ' + encoding.name); > }); Done.
Attachment #8732514 - Attachment is obsolete: true
Attachment #8732514 - Flags: review?(Ms2ger)
Attachment #8733876 - Flags: review?(Ms2ger)
Comment on attachment 8733876 [details] [diff] [review] wpt changes, v2 Review of attachment 8733876 [details] [diff] [review]: ----------------------------------------------------------------- lgtm
Attachment #8733876 - Flags: review?(Ms2ger) → review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
For the MDN writer: this patch also removed the optional parameter to the constructor, as well as changing the supported values for the encode() method.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: