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.
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
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); + }
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.
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 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
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.
(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 >
Created attachment 31398 [details] patch with a test put in manual-tests fixed a 'double' iteratorion issue pointed out by agl.
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.
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