[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see https://github.com/w3c/i18n-activity/issues/116 for more details). References to UTF-16 have been removed, and failing expectations added to the tests.

Intent to Deprecate and Remove: https://groups.google.com/a/chromium.org/d/msg/blink-dev/13uMPjRqY94/lhPLKRJnAwAJ
BUG=595351
TEST=Start chrome and run tests in third_party/WebKit/LayoutTests/fast/encoding/api and third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that relied on UTF-16 support should fail as expected.

Committed: https://crrev.com/4e0d49e0a7223958ebad85a98172ec1a8090c635
Cr-Commit-Position: refs/heads/master@{#404410}

[lpan, 2016-04-06 03:43:12]

Description was changed from

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.
==========

to

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.
==========

[lpan, 2016-04-06 03:43:12]

l.panpax@gmail.com changed reviewers:
+ jsbell@chromium.org, kbr@chromium.org

[lpan, 2016-04-06 03:43:12]

l.panpax@gmail.com changed required reviewers:
+ jsbell@chromium.org, kbr@chromium.org

[jsbell, 2016-04-06 18:03:23]

Initial feedback. I would also expect some tests under
imported/web-platform-tests/encoding to fail and need new -expected.txt files?

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/api/textencoder-labels-expected.txt
(right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/api/textencoder-labels-expected.txt:50:
FAIL "utf-16be" => "utf-16be" Failed to construct 'TextEncoder': The encoding
provided ('utf-16be') is not 'utf-8'.
Since this test is not imported, you should fix the test rather than adding an
expectation file with FAIL lines.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip-expected.txt
(right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip-expected.txt:3:
FAIL utf-16le - encode/decode round trip Failed to construct 'TextEncoder': The
encoding provided ('utf-16le') is not 'utf-8'.
Since this test is not imported, you should fix the test rather than adding an
expectation file with FAIL lines.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/encoding/TextEncoder.cpp (right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:35: // #include
"core/frame/UseCounter.h"
Delete, don't comment it out.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:42: TextEncoder*
TextEncoder::create(ExecutionContext* context, const String& utfLabel,
ExceptionState& exceptionState)
This should be updated to not have utfLabel as a parameter at all (and update
header too, obviously).

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:44: WTF::TextEncoding
encoding(utfLabel.stripWhiteSpace(&Encoding::isASCIIWhiteSpace));
This can be just:

WTF::TextEncoding encoding("UTF-8");

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:45: if
(!encoding.isValid()) {
This block can be removed.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:50: String
name(encoding.name());
This block can be removed.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:56: return new
TextEncoder(encoding);
The TextEncoding construction can even be done inline here.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:62: {
Might as well add an assert here:

ASSERT(m_encoding.name() == "UTF-8");

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/encoding/TextEncoder.idl (right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.idl:33:
Constructor(DOMString utfLabel),
This should match https://encoding.spec.whatwg.org/#interface-textencoder and
have no parameters.

[jsbell, 2016-04-08 18:36:27]

Updates to the web-platform-tests have landed. If we do another import we'll
bring in "failures" which this CL would then fix. So we should probably do that
first so this is cleaner/more obvious.

[lpan, 2016-04-09 01:10:16]

On 2016/04/08 18:36:27, jsbell wrote:
> Updates to the web-platform-tests have landed. If we do another import we'll
> bring in "failures" which this CL would then fix. So we should probably do
that
> first so this is cleaner/more obvious.

Does this mean I need to re-sync the master branch and then rebase master?

[lpan, 2016-04-09 03:25:15]

l.panpax@gmail.com changed reviewers:
- kbr@chromium.org

[lpan, 2016-04-09 03:25:15]

l.panpax@gmail.com changed required reviewers:
- kbr@chromium.org

[lpan, 2016-04-09 03:25:16]

Response to initial review.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/api/textencoder-labels-expected.txt
(right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/api/textencoder-labels-expected.txt:50:
FAIL "utf-16be" => "utf-16be" Failed to construct 'TextEncoder': The encoding
provided ('utf-16be') is not 'utf-8'.
On 2016/04/06 18:03:22, jsbell wrote:
> Since this test is not imported, you should fix the test rather than adding an
> expectation file with FAIL lines.

Partially done (fixed the label tests). However due to the changes in
TextEncoder.cpp, the assert_throws() now fails for the bad_whitespace tests (see
http://pastebin.com/8bpkygPw). I'm not sure how to address this - please advise.
And again, I can also separate the utf8 and non utf8 tests to create cleaner
output messages.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip-expected.txt
(right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip-expected.txt:3:
FAIL utf-16le - encode/decode round trip Failed to construct 'TextEncoder': The
encoding provided ('utf-16le') is not 'utf-8'.
On 2016/04/06 18:03:22, jsbell wrote:
> Since this test is not imported, you should fix the test rather than adding an
> expectation file with FAIL lines.

Done. I can separate the test functions if the output messages are too messy
(see http://pastebin.com/5dUppZrg).

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/encoding/TextEncoder.cpp (right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:35: // #include
"core/frame/UseCounter.h"
On 2016/04/06 18:03:22, jsbell wrote:
> Delete, don't comment it out.

Done.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:42: TextEncoder*
TextEncoder::create(ExecutionContext* context, const String& utfLabel,
ExceptionState& exceptionState)
On 2016/04/06 18:03:23, jsbell wrote:
> This should be updated to not have utfLabel as a parameter at all (and update
> header too, obviously).

Done.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:44: WTF::TextEncoding
encoding(utfLabel.stripWhiteSpace(&Encoding::isASCIIWhiteSpace));
On 2016/04/06 18:03:23, jsbell wrote:
> This can be just:
> 
> WTF::TextEncoding encoding("UTF-8");
> 

Done, but I think this change messes with the "bad whitespace" tests in
/LayoutTests/fast/encoding/api/textencoder-labels.html (see
http://pastebin.com/gLY5MQTi).

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:45: if
(!encoding.isValid()) {
On 2016/04/06 18:03:22, jsbell wrote:
> This block can be removed.

Done.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:50: String
name(encoding.name());
On 2016/04/06 18:03:23, jsbell wrote:
> This block can be removed.

Done.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:56: return new
TextEncoder(encoding);
On 2016/04/06 18:03:22, jsbell wrote:
> The TextEncoding construction can even be done inline here.

Acknowledged.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:62: {
On 2016/04/06 18:03:23, jsbell wrote:
> Might as well add an assert here:
> 
> ASSERT(m_encoding.name() == "UTF-8");
> 

Done.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/encoding/TextEncoder.idl (right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.idl:33:
Constructor(DOMString utfLabel),
On 2016/04/06 18:03:23, jsbell wrote:
> This should match https://encoding.spec.whatwg.org/#interface-textencoder and
> have no parameters.

Done.

[jsbell, 2016-04-11 17:23:23]

Description was changed from

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.
==========

to

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/13uMPjRqY94/lhPLKRJn...
BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.
==========

[jsbell, 2016-04-11 17:24:17]

I'm out of the office for a couple of days, so there will be a delay in
providing feedback - sorry!

[jsbell, 2016-04-13 17:52:35]

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/api/textencoder-labels-expected.txt
(right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/api/textencoder-labels-expected.txt:50:
FAIL "utf-16be" => "utf-16be" Failed to construct 'TextEncoder': The encoding
provided ('utf-16be') is not 'utf-8'.
On 2016/04/09 03:25:15, l.panpax wrote:
> On 2016/04/06 18:03:22, jsbell wrote:
> > Since this test is not imported, you should fix the test rather than adding
an
> > expectation file with FAIL lines.
> 
> Partially done (fixed the label tests). However due to the changes in
> TextEncoder.cpp, the assert_throws() now fails for the bad_whitespace tests
(see
> http://pastebin.com/8bpkygPw). I'm not sure how to address this - please
advise.
> And again, I can also separate the utf8 and non utf8 tests to create cleaner
> output messages.

You'll need to rework the test.
https://github.com/inexorabletash/text-encoding/commit/5e356a1a3b9c82de9f2e8a...
may provide some inspiration (it's the corresponding change I made to a
polyfill/"reference implementation", including tests)

So yes, separating utf-8 and non-utf-8 probably makes sense.

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/encoding/TextEncoder.cpp (right):

https://codereview.chromium.org/1862453003/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:44: WTF::TextEncoding
encoding(utfLabel.stripWhiteSpace(&Encoding::isASCIIWhiteSpace));
On 2016/04/09 03:25:16, l.panpax wrote:
> On 2016/04/06 18:03:23, jsbell wrote:
> > This can be just:
> > 
> > WTF::TextEncoding encoding("UTF-8");
> > 
> 
> Done, but I think this change messes with the "bad whitespace" tests in
> /LayoutTests/fast/encoding/api/textencoder-labels.html (see
> http://pastebin.com/gLY5MQTi).

That test becomes meaningless since TextEncoder no longer takes an argument, so
it would never throw. That whole file can just be removed.

[lpan, 2016-04-18 13:31:30]

Patchset #2 (id:20001) has been deleted

[lpan, 2016-04-18 13:41:18]

Patchset #2 (id:40001) has been deleted

[lpan, 2016-04-18 14:14:36]

PTAL before I rebase to get the new imported tests.

> That test becomes meaningless since TextEncoder no longer takes an argument,
so
> it would never throw. That whole file can just be removed.

I've assumed you were referring to fast/encoding/api/textencoder-labels.html, so
it has been deleted. This left fast/encoding/api/utf-round-trip.html as the only
other relevant non-imported test, for which I've split the utf-8 and utf-16
tests (if you were wondering why I have made so little changes to the tests).

P.S. Sorry for the slower progress! Been busy with work and elsewhere.

[jsbell, 2016-04-26 22:57:00]

Sorry about my lack of activity - I owe you replies here, and will get to it
when I can.

In the meantime, the web platform tests have been updated :)

[jsbell, 2016-05-02 20:20:07]

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html
(right):

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:33: 
nit: remove this extra whitespace; one or two blank lines at most.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:35:
utf_encodings.forEach(function(encoding) {
Maybe just split this into a test for utf-8 and then ['utf-16le',
'utf-16be'].forEach(...), since there's nothing in common?

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:36: var
utf16_exception;
this is unused - remove?

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:37: if
(encoding == "utf-8") {
nit: prefer ===

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:41: var
encoded = new TextEncoder(encoding).encode(string);
Don't pass argument to TextEncoder

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:47: else {
nit: format as `} else {`

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:51: var
encoded = new TextEncoder(encoding).encode(string);
Since encoding is ignored, how is this test intended to work?
Do you want a manual utf-16 encoder (like the utf-8 one below) instead?

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/encoding/TextEncoder.cpp (right):

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:31: #include
"modules/encoding/TextEncoder.h"
Keep the blank line below here; we separate the include of "this file's" header
from the others.

[lpan, 2016-05-03 13:42:01]

I ran rebase-update a few days ago. My apologies since there are still
outstanding comments. Fortunately the only conflict was in an xxx-expected file,
which was made redundant now that the test is fixed upstream.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html
(right):

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:35:
utf_encodings.forEach(function(encoding) {
On 2016/05/02 20:20:06, jsbell wrote:
> Maybe just split this into a test for utf-8 and then ['utf-16le',
> 'utf-16be'].forEach(...), since there's nothing in common?

Acknowledged. Will do once I figure out how to handle utf-16 encoding manually.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:36: var
utf16_exception;
On 2016/05/02 20:20:06, jsbell wrote:
> this is unused - remove?

Done.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:37: if
(encoding == "utf-8") {
On 2016/05/02 20:20:06, jsbell wrote:
> nit: prefer ===

Acknowledged.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:41: var
encoded = new TextEncoder(encoding).encode(string);
On 2016/05/02 20:20:06, jsbell wrote:
> Don't pass argument to TextEncoder

Acknowledged.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:47: else {
On 2016/05/02 20:20:06, jsbell wrote:
> nit: format as `} else {`

Acknowledged.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:51: var
encoded = new TextEncoder(encoding).encode(string);
On 2016/05/02 20:20:06, jsbell wrote:
> Since encoding is ignored, how is this test intended to work?
> Do you want a manual utf-16 encoder (like the utf-8 one below) instead?

This was my (poorly thought out) attempt at showing that a round-trip
encode/decode was no longer possible. With a manual utf-16 encoder, am I
validating that TextDecoder still works with utf-16? If so, could I not just
provide a sample utf-16 string?

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/encoding/TextEncoder.cpp (right):

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:31: #include
"modules/encoding/TextEncoder.h"
On 2016/05/02 20:20:06, jsbell wrote:
> Keep the blank line below here; we separate the include of "this file's"
header
> from the others.

Done.

[jsbell, 2016-05-03 22:22:49]

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html
(right):

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:35:
utf_encodings.forEach(function(encoding) {
On 2016/05/03 13:42:00, lpan wrote:
> On 2016/05/02 20:20:06, jsbell wrote:
> > Maybe just split this into a test for utf-8 and then ['utf-16le',
> > 'utf-16be'].forEach(...), since there's nothing in common?
> 
> Acknowledged. Will do once I figure out how to handle utf-16 encoding
manually.

function encode_utf16(s, littleEndian) {
  var a = new Uint8Array(s.length * 2), view = new DataView(a.buffer);
  s.split('').forEach(function(c, i) {
    view.setUint16(i * 2, c.charCodeAt(0), littleEndian);
  });
  return a;
}
... although that doesn't force the input to be a USVString so it doesn't behave
identically to the way the encoder used to.

https://codereview.chromium.org/1862453003/diff/60001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/encoding/api/utf-round-trip.html:51: var
encoded = new TextEncoder(encoding).encode(string);
On 2016/05/03 13:42:00, lpan wrote:
> On 2016/05/02 20:20:06, jsbell wrote:
> > Since encoding is ignored, how is this test intended to work?
> > Do you want a manual utf-16 encoder (like the utf-8 one below) instead?
> 
> This was my (poorly thought out) attempt at showing that a round-trip
> encode/decode was no longer possible. With a manual utf-16 encoder, am I
> validating that TextDecoder still works with utf-16? If so, could I not just
> provide a sample utf-16 string?

Right, it's no longer round-trip, so maybe look to see if utf-16 decode is
exercised elsewhere and if so just remove it here.

From a quick check: it's exercised (with samples) in
imported/web-platform-tests/encoding fairly well, including edge cases. So...
you're probably okay removing it if fixing it up is problematic.

[lpan, 2016-05-31 13:09:12]

I'm very sorry for going AWOL on you. This bug fix sorta fell to the bottom of
the heap with the things going around me. I know this is not a complicated bug
fix - this (chromium, c++, string encoding) was all new to me when I started.

With that said, I will strive to finish this "bug" off.

Just a few things:
1. I have rebased since it's been a few weeks (almost a month). I understand the
etiquette is to rebase separately, but it felt unnecessary.

2. I have split the utf-8 & utf-16 tests in utf-round-trip, for as much as it
made sense to me. Thanks for providing the manual encoding for utf-16 - I don't
think I would have figured it out.

3. The imported tests in third_party\WebKit\LayoutTests\ have disappeared
altogether after the resync. I assume they've been absorbed upstream somewhere?

https://codereview.chromium.org/1862453003/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/encoding/TextEncoder.cpp (right):

https://codereview.chromium.org/1862453003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:52: DCHECK_EQ(name ==
"UTF-8");
ASSERT deprecated in favour of DCHECK.

https://codereview.chromium.org/1862453003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/encoding/TextEncoder.cpp:62: DCHECK_EQ(name ==
"utf-8");
ASSERT deprecated in favour of DCHECK.

[lpan, 2016-05-31 13:29:37]

Patchset #3 (id:80001) has been deleted

[jsbell, 2016-05-31 16:53:17]

On 2016/05/31 13:09:12, lpan wrote:

> 3. The imported tests in third_party\WebKit\LayoutTests\ have disappeared
> altogether after the resync. I assume they've been absorbed upstream
somewhere?

Just moved to imported/wpt

[lpan, 2016-06-02 14:07:20]

On 2016/05/31 16:53:17, jsbell wrote:
> On 2016/05/31 13:09:12, lpan wrote:
> 
> > 3. The imported tests in third_party\WebKit\LayoutTests\ have disappeared
> > altogether after the resync. I assume they've been absorbed upstream
> somewhere?
> 
> Just moved to imported/wpt

Thanks, I'll take a look to make sure the other tests are up to snuff.

[lpan, 2016-06-06 14:50:01]

On 2016/06/02 14:07:20, lpan wrote:
> On 2016/05/31 16:53:17, jsbell wrote:
> > On 2016/05/31 13:09:12, lpan wrote:
> > 
> > > 3. The imported tests in third_party\WebKit\LayoutTests\ have disappeared
> > > altogether after the resync. I assume they've been absorbed upstream
> > somewhere?
> > 
> > Just moved to imported/wpt
> 
> Thanks, I'll take a look to make sure the other tests are up to snuff.

Tests in imported/wpt/encoding/:
api-basics.html - looks good, but I had to fix the testharness paths.
api-surrogate-utf8.html - still calls the old TextEncoder constructor (with arg)
textencoder-constructor-non-utf - still calls the old TextEncoder constructor
(with arg)
textencoder-utf16-surrogates - still calls the old TextEncoder constructor (with
arg)

Had a look at a few others too. Many have slightly incorrect <script> paths i.e.
"resource/testharness.js", when it should be "../resource/testharness.js"

PTAL before we move any further.

[lpan, 2016-06-06 14:51:31]

> PTAL before we move any further.
*At the current patchset (#3)

[jsbell, 2016-06-07 20:35:13]

Code and non-imported test changes still l*g*t*m.

> 
> Tests in imported/wpt/encoding/:
> api-basics.html - looks good, but I had to fix the testharness paths.

You should run them with:

third_party/WebKit/Tools/Scripts/run-webkit-tests imported/wpt/encoding

(You may need to specify --target=Debug or --target=Default depending on your
out dir)

That test runner handles mapping the absolute paths.

> api-surrogate-utf8.html - still calls the old TextEncoder constructor (with
arg)
> textencoder-constructor-non-utf - still calls the old TextEncoder constructor
> (with arg)
> textencoder-utf16-surrogates - still calls the old TextEncoder constructor
(with
> arg)

Those should be valid as-is - they all pass 'utf-8' - but I filed
https://github.com/w3c/web-platform-tests/pull/3129 to fix them upstream.

> Had a look at a few others too. Many have slightly incorrect <script> paths
i.e.
> "resource/testharness.js", when it should be "../resource/testharness.js"

https://codereview.chromium.org/1862453003/diff/100001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/imported/wpt/encoding/textencoder-constructor-non-utf-expected.txt
(left):

https://codereview.chromium.org/1862453003/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/imported/wpt/encoding/textencoder-constructor-non-utf-expected.txt:81:
Harness: the test ran to completion.
Huh, why did this line disappear?

Probably needs to be regenerated.

[jsbell, 2016-06-17 09:09:13]

On 2016/06/07 20:35:13, jsbell (OOO until 6-20) wrote:
> > api-surrogate-utf8.html - still calls the old TextEncoder constructor (with
> arg)
> > textencoder-constructor-non-utf - still calls the old TextEncoder
constructor
> > (with arg)
> > textencoder-utf16-surrogates - still calls the old TextEncoder constructor
> (with
> > arg)
> 
> Those should be valid as-is - they all pass 'utf-8' - but I filed
> https://github.com/w3c/web-platform-tests/pull/3129 to fix them upstream.

Merged this - now we need to do a wpt import.

(Thanks for sticking with this!)

[lpan, 2016-06-19 14:59:04]

Patchset #4 (id:120001) has been deleted

[lpan, 2016-06-20 11:24:51]

for Patch Set 5

> Merged this - now we need to do a wpt import.
> 
> (Thanks for sticking with this!)

I don't know what the workflow looks like with web-platform-tests, or how
changes over there make it back to Chromium. I'm just going to assume that when
you give me the heads up and I rebase, that it pulls those fixes from upstream
into my local repo.

>
third_party/WebKit/LayoutTests/imported/wpt/encoding/textencoder-constructor-non-utf-expected.txt:81:
> Harness: the test ran to completion.
> Huh, why did this line disappear?

>Probably needs to be regenerated.

I ran "run-webkit-tests" and it looks like
"/imported/wpt/encoding/textencoder-constructor-non-utf" was fixed, so it no
longer requires the failing expectations, therefore I've removed it.

Is there anything else I need to do or check before we push this one home?

[jsbell, 2016-06-20 18:53:21]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-20 18:54:01]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862453003/160001

[jsbell, 2016-06-20 18:54:46]

On 2016/06/20 11:24:51, lpan wrote:
> I don't know what the workflow looks like with web-platform-tests, or how
> changes over there make it back to Chromium. I'm just going to assume that
when
> you give me the heads up and I rebase, that it pulls those fixes from upstream
> into my local repo.

Correct. I'll try and take care of this. (I was out of the office last week,
should have a chance this week unless someone beats me to it.)

[commit-bot: I haz the power, 2016-06-20 20:21:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-20 20:21:12]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lpan, 2016-06-23 10:53:33]

On 2016/06/20 20:21:12, commit-bot: I haz the power wrote:
> Dry run: Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Looks like the web-kit tests failed (with and without the patch). What does this
mean?

[lpan, 2016-06-23 11:00:26]

On 2016/06/23 10:53:33, lpan wrote:
> On 2016/06/20 20:21:12, commit-bot: I haz the power wrote:
> > Dry run: Try jobs failed on following builders:
> >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> 
> Looks like the web-kit tests failed (with and without the patch). What does
this
> mean?

webkit_tests (with patch) webkit_tests (with patch) 
unexpected_failures:
fast/repaint/window-resize-background-image-fixed-centered-composited.html
virtual/threaded/fast/compositorworker/compositor-proxy-supports.html
virtual/gpu/fast/canvas/canvas-composite-repaint-by-all-imagesource.html
===========================================================================
webkit_tests (without patch) webkit_tests (without patch) 
unexpected_failures:
fast/repaint/window-resize-background-image-fixed-centered-composited.html
virtual/gpu/fast/canvas/canvas-composite-repaint-by-all-imagesource.html

The odd one out is
"virtual/threaded/fast/compositorworker/compositor-proxy-supports.html", which
looks to have been fixed.

[jsbell, 2016-06-23 19:14:12]

On 2016/06/23 11:00:26, lpan wrote:
> On 2016/06/23 10:53:33, lpan wrote:
> > On 2016/06/20 20:21:12, commit-bot: I haz the power wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
> > 
> > Looks like the web-kit tests failed (with and without the patch). What does
> this
> > mean?

Likely some other patch landed which broke those temporarily. It's probably been
fixed since.

[jsbell, 2016-06-23 19:16:15]

lgtm

[jsbell, 2016-06-23 19:16:23]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-06-23 19:17:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1862453003/160001

[commit-bot: I haz the power, 2016-06-23 20:34:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-23 20:34:02]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jsbell, 2016-06-23 22:44:14]

jsbell@chromium.org changed reviewers:
+ jbroman@chromium.org

[jsbell, 2016-06-23 22:44:15]

On 2016/06/23 20:34:02, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

Hah, the failure is real:

third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html

    // This is a hack to get a 16-bit string for a supported property.
    var transform16 = new TextDecoder('utf-16').decode(new
TextEncoder('utf-16').encode('transform'));

jbroman: we're removing support for passing an argument to TextEncoder(). Can
you explain what this is doing here? "hack" doesn't inspire confidence - this
should be a no-op.

[jbroman, 2016-06-23 22:50:02]

On 2016/06/23 at 22:44:15, jsbell wrote:
> On 2016/06/23 20:34:02, commit-bot: I haz the power wrote:
> > Try jobs failed on following builders:
> >   mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
> >
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
> 
> Hah, the failure is real:
> 
>
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-supports.html
> 
>     // This is a hack to get a 16-bit string for a supported property.
>     var transform16 = new TextDecoder('utf-16').decode(new
TextEncoder('utf-16').encode('transform'));
> 
> jbroman: we're removing support for passing an argument to TextEncoder(). Can
you explain what this is doing here? "hack" doesn't inspire confidence - this
should be a no-op.

"hack" is not supposed to inspire confidence. :D

It's logically a no-op, but not internally. WTF::String (as well as V8 strings)
can be represented as a 16-bit string, or as an 8-bit string (if each character
is in Latin-1). It so happens that TextDecoder always produces a 16-bit string
when decoding UTF-16.

I needed a way to get a 16-bit string that corresponded to an existing property.
Many places in Blink canonicalize 16-bit strings that fit in 8 bits, but not all
of them do, and this was the only way I could find to easily to it from
JavaScript. Maybe there's another way, or maybe we should have an Internals API
to get particular weird strings, dunno.

Previously there was an issue whereby the string comparison was only made if the
provided string was 8-bit, which is incorrect in cases like this.

[jsbell, 2016-06-23 22:53:39]

Okay - I was hoping this was maybe obsolete. :) Thanks for clarifying jbroman@!

Since the "magic" is in TextDecoder, we can replace the encode part with
encode_utf16() (defined higher up in the comments here) or just do:

var transform16 = new TextDecoder('utf-16').decode(new Uint8Array([116, 0, 114,
0, 97, 0, 110, 0, 115, 0, 102, 0, 111, 0, 114, 0, 109, 0]));

lpan@ - can you make that change to the test? Sorry about this. :P We're almost
done!

[lpan, 2016-06-24 13:05:52]

On 2016/06/23 22:53:39, jsbell wrote:
> Okay - I was hoping this was maybe obsolete. :) Thanks for clarifying
jbroman@!
> 
> Since the "magic" is in TextDecoder, we can replace the encode part with
> encode_utf16() (defined higher up in the comments here) or just do:
> 
> var transform16 = new TextDecoder('utf-16').decode(new Uint8Array([116, 0,
114,
> 0, 97, 0, 110, 0, 115, 0, 102, 0, 111, 0, 114, 0, 109, 0]));
> 
> lpan@ - can you make that change to the test? Sorry about this. :P We're
almost
> done!

Done - added encode_utf16() to the compositor test. Ran the test with the webkit
script and shows as good.

[jsbell, 2016-06-24 15:28:37]

On 2016/06/24 13:05:52, lpan wrote:
> Done - added encode_utf16() to the compositor test. Ran the test with the
webkit
> script and shows as good.

Can you add the // Hack comment back in? Otherwise, l*g*t*m

[lpan, 2016-06-24 22:09:24]

Patchset #6 (id:180001) has been deleted

[lpan, 2016-06-24 22:11:25]

On 2016/06/24 15:28:37, jsbell (OOO until July 7) wrote:
> On 2016/06/24 13:05:52, lpan wrote:
> > Done - added encode_utf16() to the compositor test. Ran the test with the
> webkit
> > script and shows as good.
> 
> Can you add the // Hack comment back in? Otherwise, l*g*t*m

Restored the comment and submitted and the patch set again. PTAL.

[jsbell, 2016-07-07 16:49:45]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-07-07 16:50:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jsbell, 2016-07-07 16:53:35]

still lgtm, thanks for your patience (I've been on vacation)

[commit-bot: I haz the power, 2016-07-07 17:00:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-07 17:00:57]

Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)

[lpan, 2016-07-08 15:38:06]

On 2016/07/07 16:53:35, jsbell (OOO until July 7) wrote:
> still lgtm, thanks for your patience (I've been on vacation)

No problem - hope you had a good one!



Dry run: Try jobs failed on following builders:
  ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
-------------------------------------------------------------------------------------

With these latest build errors (ios-device), I can't actually tell what's going
wrong?

[jsbell, 2016-07-08 16:32:44]

On 2016/07/08 15:38:06, lpan wrote:
> On 2016/07/07 16:53:35, jsbell (OOO until July 7) wrote:
> > still lgtm, thanks for your patience (I've been on vacation)
> 
> No problem - hope you had a good one!
> 
> 
> 
> Dry run: Try jobs failed on following builders:
>   ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
>
-------------------------------------------------------------------------------------
> 
> With these latest build errors (ios-device), I can't actually tell what's
going
> wrong?

Just a hiccup - the builder couldn't connect to goma (distributed build system).
It's good to go.

[jsbell, 2016-07-08 16:32:54]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-07-08 16:33:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-08 16:45:00]

Description was changed from

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/13uMPjRqY94/lhPLKRJn...
BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.
==========

to

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/13uMPjRqY94/lhPLKRJn...
BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.
==========

[commit-bot: I haz the power, 2016-07-08 16:45:01]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2016-07-08 16:45:19]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-08 16:47:02]

Description was changed from

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/13uMPjRqY94/lhPLKRJn...
BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.
==========

to

==========
[Blink>TextEncoder] Removed UTF-16 support from TextEncoder API.

Removal of UTF-16 support due to lack of use cases (see
https://github.com/w3c/i18n-activity/issues/116 for more details). References to
UTF-16 have been removed, and failing expectations added to the tests.

Intent to Deprecate and Remove:
https://groups.google.com/a/chromium.org/d/msg/blink-dev/13uMPjRqY94/lhPLKRJn...
BUG=595351

TEST=Start chrome and run tests in
third_party/WebKit/LayoutTests/fast/encoding/api and
third_party/WebKit/LayoutTests/imported/web-platform-tests/encoding. Tests that
relied on UTF-16 support should fail as expected.

Committed: https://crrev.com/4e0d49e0a7223958ebad85a98172ec1a8090c635
Cr-Commit-Position: refs/heads/master@{#404410}
==========

[commit-bot: I haz the power, 2016-07-08 16:47:04]

Patchset 6 (id:??) landed as
https://crrev.com/4e0d49e0a7223958ebad85a98172ec1a8090c635
Cr-Commit-Position: refs/heads/master@{#404410}