Bug 25657 - [Chromium] Non-BMP characters are not rendered correctly
Summary: [Chromium] Non-BMP characters are not rendered correctly
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Minor
Assignee: Jungshik Shin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-08 17:12 PDT by Jungshik Shin
Modified: 2009-06-19 10:49 PDT (History)
2 users (show)

See Also:


Attachments
patch without the layout test result (2.67 KB, patch)
2009-06-11 16:12 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch with a test put in manual-tests (2.42 KB, patch)
2009-06-16 14:38 PDT, Jungshik Shin
no flags Details | Formatted Diff | Diff
patch with a test put in manual-tests (2.42 KB, patch)
2009-06-16 21:58 PDT, Jungshik Shin
levin: 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 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