| Summary: | Land tests for <rdar://problem/3277733>: \ in JavaScript mishandled when encoding is Japanese | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||
| Component: | Tools / Tests | Assignee: | Alexey Proskuryakov <ap> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | abarth, eric, hamaji, webkit.review.bot | ||||
| Priority: | P2 | ||||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Alexey Proskuryakov
2010-05-24 11:15:55 PDT
The bug itself was fixed in http://trac.webkit.org/changeset/4502. Created attachment 56906 [details]
proposed patch
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. Maybe it's because the tests use Shift-JIS? 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
(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... > 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.
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. http://trac.webkit.org/changeset/60311 might have broken Qt Linux Release > > + // 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?
|