Bug 25657 – [Chromium] Non-BMP characters are not rendered correctly

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Jungshik Shin]

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.

[Adam Langley]

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

[Jungshik Shin]

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);
+    }
 

[Adam Langley]

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.

[Jungshik Shin]

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.

[Adam Langley]

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

[Jungshik Shin]

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.

[Jungshik Shin]

(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
> 

[Jungshik Shin]

Created attachment 31398 [details]
patch with a test put in manual-tests

fixed a 'double' iteratorion issue pointed out by agl.

[David Levin]

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.

[Jungshik Shin]

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