Closed Bug 1257877 Opened 4 years ago Closed 4 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.
Blocks: encoding
Attached patch patchSplinter Review
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Attachment #8732513 - Flags: review?(hsivonen)
Attached patch wpt changes (obsolete) — 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, v2Splinter 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+
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
https://hg.mozilla.org/mozilla-central/rev/cd3799dfd531
https://hg.mozilla.org/mozilla-central/rev/fbb40278805f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
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
You need to log in before you can comment on or make changes to this bug.