Currently WebCore/rendering/RenderThemeChromiumSkia.cpp defins 'Arial' as the default GUI font,
but this is not suitable for languages that use non-latin characters, like CJK.
I'd like to propose that we would use "Arial, sans-serif" so that some proper fallback sans-serif font
could be selected for non-latin characters.
--- WebCore/rendering/RenderThemeChromiumSkia.cpp (revision 79111)
+++ WebCore/rendering/RenderThemeChromiumSkia.cpp (working copy)
@@ -79,7 +79,7 @@
// sizes (e.g. 15px). So, for now we just use Arial.
const String& RenderThemeChromiumSkia::defaultGUIFont()
- DEFINE_STATIC_LOCAL(String, fontFace, ("Arial"));
+ DEFINE_STATIC_LOCAL(String, fontFace, ("Arial, sans-serif"));
Created attachment 84206 [details]
Change the default fallback font-family from arial to arial, sans-serif
If this brings us closer to IE's behavior, I think that's a good thing.
Does this change the results of any layout tests? Can we add some layouts tests that illustrate the change?
The comment mentions that IE uses a font based on the encoding. How hard is that to do (might not be worth the effort)?
The FIXME mentions that we don't match IE for ANSI encodings. Is that still the case?
I thought Arial had CJK characters, or is that only Arial Unicode?
Thanks for the comment.
I don't have IE/Windows handy now, so I'll check later today.
Arial doesn't have CJK characters, but "Arial Unicode MS" does (but a lot bigger).
(I'll also check if IE falls back "Arial" to "Arial Unicode MS" when it
needs to render any CJK characters.)
I believe that IE (I'm not sure about IE 9) uses different fonts even for controls like 'buttons' depending on the language ('lang' specified per element/page), the page encoding (as an indirect indicator of lang) and the UI language of IE. The same is true of Firefox, IIRC.
In the past, Chrome used to have
https://bugs.webkit.org/show_bug.cgi?id=18085 and Ojan's patch (that relied on it) to emulate Firefox and IE.
Anyway, I believe adding 'sans-serif' here will certainly help at least as long as the UI language of Chrome matches that of the intended language of a page in current view.
BTW, I think this code change is fine. You need to include a ChangeLog and set the r? flag if you want it reviewed.
Also, if it's not testable, please say why this change isn't testable. I suspect you can create a manual test in WebCore/manual-tests if you're unable to make a layout test.
Created attachment 88122 [details]
Added a ChangeLog entry and a comment on the source code.
Created attachment 88123 [details]
Fixed a tab in ChangeLog.
Created attachment 88126 [details]
Clarify the wording of ChangeLog statement.
Attachment 88126 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1
Source/WebCore/rendering/RenderThemeChromiumSkia.cpp:79: Should only a single space after a punctuation in a comment. [whitespace/comments] 
Total errors found: 1 in 2 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 88138 [details]
Fix comment style.
Comment on attachment 88138 [details]
Fix comment style.
Clearing flags on attachment: 88138
Committed r82915: <http://trac.webkit.org/changeset/82915>
All reviewed patches have been landed. Closing bug.
http://trac.webkit.org/changeset/82915 might have broken GTK Linux 32-bit Debug
Reopening. This caused a bunch of tests to change results:
They will need rebaselining if the change is expected and correct.
(In reply to comment #14)
Sorry for the trouble, and for this naive question:
where can I find the information which 270 tests failed for this?
(In reply to comment #15)
> (In reply to comment #14)
> Sorry for the trouble, and for this naive question:
> where can I find the information which 270 tests failed for this?
It's on the waterfall. Specifically:
Thanks for the info!
I'll look into the failures and get back to you once everything is resolved.
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #14)
> > Sorry for the trouble, and for this naive question:
> > where can I find the information which 270 tests failed for this?
> It's on the waterfall. Specifically: