Remove support for UTF-16 from TextEncoder

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
13 days ago

People

(Reporter: hsivonen, Assigned: emk)

Tracking

(Blocks: 1 bug, {dev-doc-needed, site-compat})

unspecified
mozilla48
dev-doc-needed, site-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
https://github.com/whatwg/encoding/commit/8360f775c8df145f649047c7d59c5ff733ade112 removed support for UTF-16 in TextEncoder. We should make our implementation reflect the spec change.
(Reporter)

Updated

3 years ago
Blocks: 746911
(Assignee)

Comment 2

3 years ago
Posted patch patchSplinter Review
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8732513 - Flags: review?(hsivonen)
(Assignee)

Comment 3

3 years ago
Posted patch wpt changes (obsolete) — Splinter Review
Attachment #8732514 - Flags: review?(Ms2ger)
Keywords: dev-doc-needed
(Reporter)

Comment 4

3 years ago
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);
});
(Assignee)

Comment 6

3 years ago
(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+
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3799dfd531b8a72becc4c2c78128fd052d8818
Bug 1257877 - Remove UTF-16 support from TextEncoder. r=hsivonen

https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb40278805ff5a8aac8b1a1dbd8b0a180f82296
Bug 1257877 - Remove UTF-16 support from TextEncoder tests to reflect a spec change. r=Ms2ger

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd3799dfd531
https://hg.mozilla.org/mozilla-central/rev/fbb40278805f
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Depends on: 1259921
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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.