Text Encoding: Convert fast/encoding tests to testharness.js

Text Encoding: Convert fast/encoding tests to testharness.js

Convert a handful of js-test tests to testharness, with an eye towards
adding more and eventually upstreaming them.

R=foolip@chromium.org

Committed: https://crrev.com/d8ebb34eb879a486df380d542d2911f05f7d51d7
Cr-Commit-Position: refs/heads/master@{#422868}

[jsbell, 2016-10-03 23:21:09]

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

[commit-bot: I haz the power, 2016-10-03 23:22:01]

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

[jsbell, 2016-10-03 23:22:04]

Inspired by https://codereview.chromium.org/2379333003/ - convert tests from
js-test to testharness.js

If this lands first (likely) I'll convert the other CL's test over.

foolip@ - please take a look?

[commit-bot: I haz the power, 2016-10-04 00:31:04]

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

[commit-bot: I haz the power, 2016-10-04 00:31:05]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-10-04 09:35:18]

lgtm % nits

I don't propose to convert to TextDecoder now, just asking.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/encoding/char-encoding.html (right):

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/char-encoding.html:60: //
Previously caused an error.
This comment will probably look mysterious when upstreaming.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/resources/char-decoding-utils.js
(right):

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-decoding-utils.js:1:
function decodeText(charsetName, characterSequence) {
Can't this use https://encoding.spec.whatwg.org/#interface-textdecoder? Is the
concern that XHR and the API might use different code paths? If so test both?

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-decoding-utils.js:4:
req.open('GET', `data:text/plain,${characterSequence}`, /*async*/true);
async is the default, omit? (It helps understand this CL, but not really
afterwards.)

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js
(right):

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:22:
input.setAttribute('name', 'text');
input.name = 'text'?

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:25:
iframe.onload = e => {
e is unused, so ()?

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:26:
const url = String(iframe.contentWindow.location);
Maybe intentional, but iframe.contentWindow.location.href would also work.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:42:
t.done();
t.done() ought not be needed with promise_test?

[jsbell, 2016-10-04 17:41:39]

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
File third_party/WebKit/LayoutTests/fast/encoding/char-encoding.html (right):

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/char-encoding.html:60: //
Previously caused an error.
On 2016/10/04 09:35:18, foolip wrote:
> This comment will probably look mysterious when upstreaming.

Good point. Moved it up with the other UTF-8 cases.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/resources/char-decoding-utils.js
(right):

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-decoding-utils.js:1:
function decodeText(charsetName, characterSequence) {
On 2016/10/04 09:35:18, foolip wrote:
> Can't this use https://encoding.spec.whatwg.org/#interface-textdecoder? Is the
> concern that XHR and the API might use different code paths? If so test both?

They do differ. :( Specifically the API flushes at the end of a stream, other
cases do not.

Agreed that we should have tests that cover all encodings via all methods of
doing encode/decode (e.g. API, XHR, forms/URL, ...). On the to-do list, probably
as a follow on to the big i18n test upstreaming. Tracked as
https://bugs.chromium.org/p/chromium/issues/detail?id=409992

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-decoding-utils.js:4:
req.open('GET', `data:text/plain,${characterSequence}`, /*async*/true);
On 2016/10/04 09:35:18, foolip wrote:
> async is the default, omit? (It helps understand this CL, but not really
> afterwards.)

Done.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js
(right):

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:22:
input.setAttribute('name', 'text');
On 2016/10/04 09:35:18, foolip wrote:
> input.name = 'text'?

Thanks - leftover from investigating why iframe.name ordering mattered.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:25:
iframe.onload = e => {
On 2016/10/04 09:35:18, foolip wrote:
> e is unused, so ()?

Sure, done.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:26:
const url = String(iframe.contentWindow.location);
On 2016/10/04 09:35:18, foolip wrote:
> Maybe intentional, but iframe.contentWindow.location.href would also work.

I don't believe it was intentional. Changed.

https://codereview.chromium.org/2390083002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/encoding/resources/char-encoding-utils.js:42:
t.done();
On 2016/10/04 09:35:18, foolip wrote:
> t.done() ought not be needed with promise_test?

D'oh, was an async_test for a while. Thanks, removed.

[jsbell, 2016-10-04 17:43:02]

The CQ bit was checked by jsbell@chromium.org

[jsbell, 2016-10-04 17:43:02]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2390083002/#ps20001
(title: "Review feedback")

[commit-bot: I haz the power, 2016-10-04 17:43:46]

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-10-04 18:44:21]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-04 18:47:28]

Description was changed from

==========
Text Encoding: Convert fast/encoding tests to testharness.js

Convert a handful of js-test tests to testharness, with an eye towards
adding more and eventually upstreaming them.

R=foolip@chromium.org
==========

to

==========
Text Encoding: Convert fast/encoding tests to testharness.js

Convert a handful of js-test tests to testharness, with an eye towards
adding more and eventually upstreaming them.

R=foolip@chromium.org

Committed: https://crrev.com/d8ebb34eb879a486df380d542d2911f05f7d51d7
Cr-Commit-Position: refs/heads/master@{#422868}
==========

[commit-bot: I haz the power, 2016-10-04 18:47:29]

Patchset 2 (id:??) landed as
https://crrev.com/d8ebb34eb879a486df380d542d2911f05f7d51d7
Cr-Commit-Position: refs/heads/master@{#422868}