Closed
Bug 1257877
Opened 9 years ago
Closed 9 years ago
Remove support for UTF-16 from TextEncoder
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
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)
(deleted),
patch
|
hsivonen
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Ms2ger
:
review+
|
Details | Diff | Splinter Review |
https://github.com/whatwg/encoding/commit/8360f775c8df145f649047c7d59c5ff733ade112 removed support for UTF-16 in TextEncoder. We should make our implementation reflect the spec change.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee | ||
Comment 2•9 years ago
|
||
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8732514 -
Flags: review?(Ms2ger)
Updated•9 years ago
|
Keywords: dev-doc-needed
Reporter | ||
Comment 4•9 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 5•9 years ago
|
||
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•9 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 7•9 years ago
|
||
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•9 years ago
|
||
Assignee | ||
Comment 9•9 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•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/cd3799dfd531
https://hg.mozilla.org/mozilla-central/rev/fbb40278805f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 11•9 years ago
|
||
Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/textencoder-no-longer-supports-utf-16/
Keywords: site-compat
Comment 12•9 years ago
|
||
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.
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•