Bug 25657

Summary: [Chromium] Non-BMP characters are not rendered correctly
Product: WebKit Reporter: Jungshik Shin <jshin>
Component: Layout and RenderingAssignee: Jungshik Shin <jshin>
Status: RESOLVED FIXED    
Severity: Minor CC: agl, ap
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
patch without the layout test result
none
patch with a test put in manual-tests
none
patch with a test put in manual-tests levin: review+

Description Jungshik Shin 2009-05-08 17:12:00 PDT
In getFontForCharacters in FontCacheLinux.cpp in platform/graphics/chromium does not handle surrogate pairs for non-BMP characters properly. 

It is trivial to fix, but I need to make a simple TTF supporting a few non-BMP characters to check in for layout test.
Comment 1 Adam Langley 2009-05-08 18:25:24 PDT
There are lots of changes pending in the Linux font code. I'm waiting on the Skia unforking. Please don't try and fix anything, you'll just make merging harder!

Thanks
Comment 2 Jungshik Shin 2009-06-09 15:38:39 PDT
Adam, are you done with the Skia unforking? 

Actually, the change does not touch Skia at all. It's like this in FontCacheLinux.cpp. 

+#include <unicode/utf16.h>
 #include <wtf/Assertions.h>
 
 namespace WebCore {
@@ -59,8 +60,11 @@
                                                           int length)
 {
     FcCharSet* cset = FcCharSetCreate();
-    for (int i = 0; i < length; ++i)
-        FcCharSetAddChar(cset, characters[i]);
+    for (int i = 0; i < length; ++i) {
+        UChar32 ucs4 = 0;
+        U16_NEXT(characters, i, length, ucs4);
+        FcCharSetAddChar(cset, ucs4);
+    }
 
Comment 3 Adam Langley 2009-06-09 15:45:29 PDT
Yes, Skia is unforked. I just don't have time to work on complex text stuff right now. Feel free to land a patch if you like.
Comment 4 Jungshik Shin 2009-06-11 16:12:19 PDT
Created attachment 31175 [details]
patch without the layout test result

I have yet to make a freely-distributable font with a few non-BMP glyphs to use for layout test. Therefore, the test is attached without a result.
Comment 5 Adam Langley 2009-06-14 10:28:37 PDT
Comment on attachment 31175 [details]
patch without the layout test result

> +    for (int i = 0; i < length; ++i) {
> +        UChar32 ucs4 = 0;

Setting this to zero every time would appear to be a waste.

> +        U16_NEXT(characters, i, length, ucs4);

U16_NEXT is a terrible name for this macro! (Nothing to do with with patch though).

From looking at the macro, it appears that it increments |i| for you:
  r30377/WebKit/mac/icu/unicode/utf16.h?rev=3">https://dev.mobileread.com/trac/webkitbrowser/browser/trunk/WebKit-r30377/WebKit/mac/icu/unicode/utf16.h?rev=3
However, you are also incrementing i in the for loop. Might not you be skipping charactors?



AGL
Comment 6 Jungshik Shin 2009-06-16 14:38:33 PDT
Created attachment 31376 [details]
patch with a test put in manual-tests

I put non-bmp.html in WebCore/manual-tests. Once I make a small font covering non-BMP and contribute to the webkit, I'll move the test to fast/text.
Comment 7 Jungshik Shin 2009-06-16 14:47:30 PDT
(In reply to comment #5)


> 
> From looking at the macro, it appears that it increments |i| for you:
>  
> r30377/WebKit/mac/icu/unicode/utf16.h?rev=3">https://dev.mobileread.com/trac/webkitbrowser/browser/trunk/WebKit-r30377/WebKit/mac/icu/unicode/utf16.h?rev=3
> However, you are also incrementing i in the for loop. Might not you be skipping
> charactors?

You're right. I shouldn't have resurrected the patch from the memory. When I tested it on Linux, I used while loop. 
Thanks for catching it. 
 
> 
> AGL
> 

Comment 8 Jungshik Shin 2009-06-16 21:58:58 PDT
Created attachment 31398 [details]
patch with a test put in manual-tests

fixed a 'double' iteratorion issue pointed out by agl.
Comment 9 David Levin 2009-06-18 12:35:45 PDT
Comment on attachment 31398 [details]
patch with a test put in manual-tests

There are tabs in the changelog which will need to be fixed on landing.
Comment 10 Jungshik Shin 2009-06-19 10:49:53 PDT
Landed in r44858 ( http://trac.webkit.org/changeset/44858)
Sending        WebCore/ChangeLog
Adding         WebCore/manual-tests/non-bmp.html
Sending        WebCore/platform/graphics/chromium/FontCacheLinux.cpp