Remove support for UTF-16 from TextEncoder

RESOLVED FIXED in Firefox 48

Status

()

Core
DOM
RESOLVED FIXED
a year ago
a year 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

a year 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

a year ago
Blocks: 746911
(Assignee)

Comment 1

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=079860900d30
(Assignee)

Comment 2

a year ago
Created attachment 8732513 [details] [diff] [review]
patch
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8732513 - Flags: review?(hsivonen)
(Assignee)

Comment 3

a year ago
Created attachment 8732514 [details] [diff] [review]
wpt changes
Attachment #8732514 - Flags: review?(Ms2ger)
Keywords: dev-doc-needed
(Reporter)

Comment 4

a year 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

a year ago
Created attachment 8733876 [details] [diff] [review]
wpt changes, v2

(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 8

a year ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=201d62214f46
(Assignee)

Comment 9

a year 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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cd3799dfd531
https://hg.mozilla.org/mozilla-central/rev/fbb40278805f
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/textencoder-no-longer-supports-utf-16/
Keywords: site-compat
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.
You need to log in before you can comment on or make changes to this bug.