Bug 39606 – Land tests for <rdar://problem/3277733>: \ in JavaScript mishandled when encoding is Japanese

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Alexey Proskuryakov]

This is an ancient bug, fixed before having regression tests became a policy.

[Alexey Proskuryakov]

The bug itself was fixed in http://trac.webkit.org/changeset/4502.

[Alexey Proskuryakov]

Created attachment 56906 [details]
proposed patch

[Alexey Proskuryakov]

Note that this test behaves differently in browser - IE displays yen signs here, and it looks like bug 24906 didn't help this case.

The should be still fine for DumpRenderTree text-only mode.

[Alexey Proskuryakov]

Maybe it's because the tests use Shift-JIS?

[Shinichiro Hamaji]

Comment on attachment 56906 [details]
proposed patch

Looks good, except for a few style nitpicks.

LooksLayoutTests/fast/encoding/yentest.html:13
 +  // document.write("one backslash inline: " + "\" + "<br>");
We can remove this line?

LayoutTests/fast/encoding/resources/yentestexternal2.js:2
 +  document.write("two backslashes external: \\ <br>");
It would be better to indent lines in functions.


LayoutTests/fast/encoding/resources/yentestexternal2.js:1
 +  function writeYenExternal() {
It would be better to break line between () and { to be consistent with C++ coding style.

LayoutTests/fast/encoding/resources/yentestexternal.js:1
 +  function writeYenExternal() {
The same comment as yenexternal2.js

[Shinichiro Hamaji]

(In reply to comment #4)
> Maybe it's because the tests use Shift-JIS?

Yes. Only EUC-JP and Shift_JIS_X0213-2000 contents are transcoded for now. I believe we should add at least Shift_JIS and ISO-2022-JP families.

FYI, I've never seen a website which specifies charset=Shift_JIS_X0213-2000 and I'm guessing the person who added Shift_JIS_X0213-2000 intended to add Shift_JIS...

[Alexey Proskuryakov]

> Shift_JIS_X0213-2000 intended to add Shift_JIS...

It is also possible that at the time, encoding name was normalized by ICU, so Shift_JIS was actually covered.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/60311>.

>  +  // document.write("one backslash inline: " + "\" + "<br>");
> We can remove this line?

Addressed the comments. I usually try to keep layout tests close to original form, because we get more accidental testing that way. If we never land commented out code, how would we be sure that it's not executed? And how would we know that non-indented code in functions works well? It's rather frequent that such accidental testing helps catch a bug.

But many others disagree with that approach, and prefer to keep layout tests clean.

[WebKit Review Bot]

http://trac.webkit.org/changeset/60311 might have broken Qt Linux Release

[Shinichiro Hamaji]

> >  +  // document.write("one backslash inline: " + "\" + "<br>");
> > We can remove this line?
> 
> Addressed the comments. I usually try to keep layout tests close to original form, because we get more accidental testing that way. If we never land commented out code, how would we be sure that it's not executed? And how would we know that non-indented code in functions works well? It's rather frequent that such accidental testing helps catch a bug.

Very interesting. Usually, I'm trying to create a small and clean layout tests to make the point of a test clearer. I can also understand your opinion. Having diversity in our tests could be helpful. Maybe we should land both tests when the original report is quite different from the minimal test?