Bug 21581 - http/tests/misc/frame-default-enc-same-domain.html should not depend on the presence of x-mac-cyrillic converter
Summary: http/tests/misc/frame-default-enc-same-domain.html should not depend on the p...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Trivial
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-13 16:14 PDT by Jungshik Shin
Modified: 2008-10-22 01:10 PDT (History)
2 users (show)

See Also:


Attachments
patch (1.44 KB, patch)
2008-10-13 17:16 PDT, Jungshik Shin
ap: review-
Details | Formatted Diff | Diff
patch with windows-1256 (1.59 KB, patch)
2008-10-14 14:30 PDT, Jungshik Shin
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jungshik Shin 2008-10-13 16:14:25 PDT
http/tests/misc/frame-default-enc-same-domain.html has meta charset declaration for x-mac-cyrillic. However, the test is for the inheritance of charset from the parent frame.  Some projects using webkit (e.g. Chrome) use their own custom build of ICU without x-mac-cyrillic converter.  It's not a good idea to make a single test depend on something other than what it intends to test.
Comment 1 Jungshik Shin 2008-10-13 17:16:20 PDT
Created attachment 24333 [details]
patch 

This patch replaces x-mac-cyrillic with windows-1251
Comment 2 Darin Adler 2008-10-13 18:01:55 PDT
Comment on attachment 24333 [details]
patch 

r=me
Comment 3 Adam Barth 2008-10-14 01:19:22 PDT
Will land.
Comment 4 Adam Barth 2008-10-14 03:01:49 PDT
My Mac Mini died in the middle of testing this patch.  I'm going to take it into the store tomorrow.
Comment 5 Alexey Proskuryakov 2008-10-14 04:06:03 PDT
Comment on attachment 24333 [details]
patch 

Sorry, I specifically chose MacCyrillic to make this test work reasonably on my machine (where I have windows-1251 as a default encoding). I'd ask you to change this to some encoding that is unlikely to be user default for any WebKit developer.

On another note, I'm surprised that you can get away without supporting MacCyrillic - this encoding is in wide use on the Web. Many Russian sites re-encode their text to MacCyrillic on the fly if they see "Mac" in UA string (a practice that makes no sense in this age, but anyway). Maybe you don't get bug reports about this because you only have a Windows beta now?
Comment 6 Adam Barth 2008-10-14 12:29:28 PDT
Reassigning to Jungshik based on comment above.
Comment 7 Jungshik Shin 2008-10-14 12:45:30 PDT
(In reply to comment #5)
> (From update of attachment 24333 [details] [edit])
> Sorry, I specifically chose MacCyrillic to make this test work reasonably on my
> machine (where I have windows-1251 as a default encoding). I'd ask you to
> change this to some encoding that is unlikely to be user default for any WebKit
> developer.

Sure, I can change (to what encoding? I'll try to get the most unlikely one), but does it really matter in layout test? 

> On another note, I'm surprised that you can get away without supporting
> MacCyrillic - this encoding is in wide use on the Web. Many Russian sites
> re-encode their text to MacCyrillic on the fly if they see "Mac" in UA string
> (a practice that makes no sense in this age, but anyway). Maybe you don't get
> bug reports about this because you only have a Windows beta now?
 
I'm aware of the practice. I have little idea how widespread the practice is. mail.ru was one of them, but it does not do that any more, does it? Anyway, if the practice is still widespread, I'll add it back (before we release Mac version)  paying the price for one more item in the encoding list and a few more kBs in the ICU data file :-)

Comment 8 Alexey Proskuryakov 2008-10-14 13:04:38 PDT
(In reply to comment #7)
> Sure, I can change (to what encoding? I'll try to get the most unlikely one),
> but does it really matter in layout test? 

Yes, we try to make layout tests work in the browser if possible. This reduces confusion and simplifies debugging in case the test breaks.

ISO-8859-5 is another example of a Cyrillic encoding that is unlikely to be default for many people.

> I'm aware of the practice. I have little idea how widespread the practice is.
> mail.ru was one of them, but it does not do that any more, does it? Anyway, if
> the practice is still widespread, I'll add it back (before we release Mac
> version)  paying the price for one more item in the encoding list and a few
> more kBs in the ICU data file :-)

I don't have any current examples. Besides mail.ru, anekdot.ru used to do the same until very recently, and these are some of the largest resources out there. Also, this was the default configuration of Russian Apache distribution, so I expect that sites doing this will be around for a long time.
Comment 9 Jungshik Shin 2008-10-14 14:30:26 PDT
Created attachment 24346 [details]
patch with windows-1256

I replaced windows-1251 with windows-1256 (Arabic)
Comment 10 Jungshik Shin 2008-10-14 17:14:58 PDT
(In reply to comment #9)
> Created an attachment (id=24346) [edit]
> patch with windows-1256
> 
> I replaced windows-1251 with windows-1256 (Arabic)

My patch and Alexey's reply 'crossed in mail'. Anyway, I guess windows-1256 should be as good as ISO-8859-5.  ISO-8859-15 would have been better :-)

As for Russian Apache (mod_charset), does the current version still do that? If so, at least we should ask them to not to do it any more. 





Comment 11 Alexey Proskuryakov 2008-10-15 00:05:09 PDT
Comment on attachment 24346 [details]
patch with windows-1256

r=me

> As for Russian Apache (mod_charset), does the current version still do that? If
> so, at least we should ask them to not to do it any more. 

I used to think that it was no longer maintained, but it turns out to be not quite so. Anyway, according to documentation, the default for CharsetAgent is empty now.
Comment 12 Jungshik Shin 2008-10-21 13:46:16 PDT
Adam or Alexey, can you land the latest patch? 
Comment 13 Adam Barth 2008-10-21 15:49:30 PDT
Yep.  Assigned.
Comment 14 Adam Barth 2008-10-22 01:10:54 PDT
Fixed in http://trac.webkit.org/changeset/37775