http://www.unicode.org/Public/UNIDATA/StandardizedVariants.html http://www.unicode.org/ivd/index.html It would be nice to support Unicode variation selector (UVS). See the above URL for information about UVS. MacOS 10.6, Windows 7 and FreeType2 support UVS and Firefox 4 beta can render UVS correctly.
Created attachment 76844 [details] Patch V0
(In reply to comment #1) > Created an attachment (id=76844) [details] > Patch V0 Hi, I wrote a patch for this. This patch makes WebKit possible to render UVSes in MacOS10.6. Here are some notes: - For win port with CoreGraphics wrapper, I tried to use wkGetGlyphs() to obtain glyph id for UVSes, but it seems that it doesn't support UVSes even on Windows7, which supports UVSes. Since I cannot see the code and cannot modify it, I think we should wait for updating the library function by Apple. - Other ports are not supported for now. Once I can successfully add UVSes support on MacOS10.6, I'd like to continue to implement on another ports which provide a way to handle UVSes (e.g. ports using FreeType2). - For a test, I used a tiny TrueType font which is used by Mozilla <http://mxr.mozilla.org/mozilla-central/source/layout/reftests/fonts/gw432047.ttf>. It's originally from <http://en.glyphwiki.org/> and can be used in public (See the license: http://en.glyphwiki.org/wiki/GlyphWiki:License).
BTW, I uses wkGetGlyphsForCharacters() to obtain glyph ID of UVSes. It generally works well, but it sometimes causes crash, even if it seems that the patch passes valid arguments to it. The function is provided as a library function, so I can't investigate the cause. Here is a stack trace when it crashed: Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x000000011ddb0e2a 0x00007fff8462f1f6 in std::equal_range<TFormat14UVSTable::UnicodeValueRange const*, int> () (gdb) bt 30 #0 0x00007fff8462f1f6 in std::equal_range<TFormat14UVSTable::UnicodeValueRange const*, int> () #1 0x00007fff8462a30c in TFormat14UVSTable::Map () #2 0x00007fff8462ffc3 in TFormat12UTF16cmapTable::MapT<true> () #3 0x00007fff845f881f in TcmapUnicodeTable::Map () #4 0x00007fff845f7a2d in TSFNTFont::GetGlyphsPerCharacters () #5 0x00007fff845f7931 in FPFontGetGlyphsForUnichars () #6 0x00000001018d63e8 in WebCore::GlyphPage::glyphDataForCharacterWithUVS (this=0x1092c4600, character=8745, selector=65024) at /Users/bashi/webkit/fonts/WebCore/platform/graphics/mac/GlyphPageTreeNodeMac.cpp:183 #7 0x000000010187b673 in WebCore::Font::glyphDataForCharacterWithUVS (this=0x107479d48, character=8745, selector=65024) at /Users/bashi/webkit/fonts/WebCore/platform/graphics/FontFastPath.cpp:218 #8 0x0000000102195a69 in WebCore::WidthIterator::advance (this=0x7fff5fbfa310, offset=3, glyphBuffer=0x0) at /Users/bashi/webkit/fonts/WebCore/platform/graphics/WidthIterator.cpp:131 #9 0x000000010187a40c in WebCore::Font::floatWidthForSimpleText (this=0x107479d48, run=@0x7fff5fbfa450, glyphBuffer=0x0, fallbackFonts=0x0, glyphOverflow=0x0) at /Users/bashi/webkit/fonts/WebCore/platform/graphics/FontFastPath.cpp:439 #10 0x0000000101869e08 in WebCore::Font::floatWidth (this=0x107479d48, run=@0x7fff5fbfa450, fallbackFonts=0x0, glyphOverflow=0x0) at /Users/bashi/webkit/fonts/WebCore/platform/graphics/Font.cpp:191 #11 0x000000010159a68f in WebCore::Font::width (this=0x107479d48, run=@0x7fff5fbfa450, fallbackFonts=0x0, glyphOverflow=0x0) at Font.h:98 #12 0x0000000101e63a90 in WebCore::textWidth (text=0x107496728, from=12, len=3, font=@0x107479d48, xPos=230, isFixedPitch=false, collapseWhiteSpace=true) at /Users/bashi/webkit/fonts/WebCore/rendering/RenderBlockLineLayout.cpp:1383 #13 0x0000000101e67204 in WebCore::RenderBlock::findNextLineBreak (this=0x107497a38, resolver=@0x7fff5fbfaa00, firstLine=true, isLineEmpty=@0x7fff5fbfadf5, previousLineBrokeCleanly=@0x7fff5fbfadf9, hyphenated=@0x7fff5fbfadf3, clear=0x7fff5fbfad98, lastFloatFromPreviousLine=0x0) at /Users/bashi/webkit/fonts/WebCore/rendering/RenderBlockLineLayout.cpp:1877 #14 0x0000000101e6aea5 in WebCore::RenderBlock::layoutInlineChildren (this=0x107497a38, relayoutChildren=false, repaintLogicalTop=@0x7fff5fbfb19c, repaintLogicalBottom=@0x7fff5fbfb198) at /Users/bashi/webkit/fonts/WebCore/rendering/RenderBlockLineLayout.cpp:667 #15 0x0000000101e4b4f0 in WebCore::RenderBlock::layoutBlock (this=0x107497a38, relayoutChildren=false, pageLogicalHeight=0) at /Users/bashi/webkit/fonts/WebCore/rendering/RenderBlock.cpp:1206 (snip)
Comment on attachment 76844 [details] Patch V0 View in context: https://bugs.webkit.org/attachment.cgi?id=76844&action=review > JavaScriptCore/wtf/unicode/Unicode.h:42 > +#define U16_IS_UVS(selector) \ [drive-by comments from a non-reviewer] These three lines should be joined to be one line.
Hi, Could anyone could review this?
Ping? (In reply to comment #5) > Hi, > > Could anyone could review this?
Comment on attachment 76844 [details] Patch V0 I have no knowledge to say r+, but this patch is not acceptable. - We prefer inline functions to macros. - We should add a test to Skipped file or test_expectations.txt if we know the test will fail. - unicode-variation-selector.html is not reviewable. diff --git a/LayoutTests/fast/text/unicode-variation-selector.html b/LayoutTests/fast/text/unicode-variation-selector.html new file mode 100644 index 0000000000000000000000000000000000000000..473131a564f46aade7620ee06a06532c4fc5835a GIT binary patch literal 1848
Created attachment 98478 [details] Patch V1
Hi Kent-san, Thank you for the review. I've addressed your comments and changed the implementation slightly. I'd be glad if someone who is familiar with this area could take a look of the patch. (In reply to comment #7) > (From update of attachment 76844 [details]) > I have no knowledge to say r+, but this patch is not acceptable. > > - We prefer inline functions to macros. > - We should add a test to Skipped file or test_expectations.txt if we know the test will fail. > - unicode-variation-selector.html is not reviewable. > > diff --git a/LayoutTests/fast/text/unicode-variation-selector.html b/LayoutTests/fast/text/unicode-variation-selector.html > new file mode 100644 > index 0000000000000000000000000000000000000000..473131a564f46aade7620ee06a06532c4fc5835a > GIT binary patch > literal 1848
Comment on attachment 98478 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=98478&action=review > LayoutTests/ChangeLog:18 > + * platform/gtk/Skipped: Skip fast/text/unicode-variation-selector.html > + * platform/mac/fast/text/unicode-variation-selector-expected.png: Added. > + * platform/mac/fast/text/unicode-variation-selector-expected.txt: Added. > + * platform/qt/Skipped: Skip fast/text/unicode-variation-selector.html > + * platform/win/Skipped: Ditto. Don't you need to update platform/chromium/test_expectation.txt? > Source/WebCore/platform/graphics/WidthIterator.cpp:52 > + static const UChar trailStart = (0xDC00 | ((0xE0100-0x10000) & 0x3FF)); > + static const UChar trailEnd = (0xDC00 | ((0xE01EF-0x10000) & 0x3FF)); Need whitespace around '-' > Source/WebCore/platform/graphics/WidthIterator.cpp:149 > + if (currentCharacter < offset - 1 && isUnicodeVariationSelector(*(cp+1))) { > + fontData->updateGlyphWithVariationSelector(c, *(cp+1), glyph); > + clusterLength += 1; > + } else if (currentCharacter < offset - 2 && isIdeographicVariationSelector(*(cp+1), *(cp+2))) { > + fontData->updateGlyphWithVariationSelector(c, U16_GET_SUPPLEMENTARY(*(cp+1), *(cp+2)), glyph); Need whitespace around '+'. BTW, you can write them as cp[1] or cp[2].
Comment on attachment 98478 [details] Patch V1 Attachment 98478 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8939326 New failing tests: fast/text/unicode-variation-selector.html
Created attachment 98482 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Created attachment 98585 [details] Patch V2
Comment on attachment 98478 [details] Patch V1 View in context: https://bugs.webkit.org/attachment.cgi?id=98478&action=review Hi Kent-san, Thank you for review. As of r89733, WidthIterator uses SurrogatePairAwareTextIterator to iterate characters so I've revised the patch to follow the change. >> LayoutTests/ChangeLog:18 >> + * platform/win/Skipped: Ditto. > > Don't you need to update platform/chromium/test_expectation.txt? I've added the expectation, but I'm not sure that is correct... >> Source/WebCore/platform/graphics/WidthIterator.cpp:52 >> + static const UChar trailEnd = (0xDC00 | ((0xE01EF-0x10000) & 0x3FF)); > > Need whitespace around '-' Done. >> Source/WebCore/platform/graphics/WidthIterator.cpp:149 >> + fontData->updateGlyphWithVariationSelector(c, U16_GET_SUPPLEMENTARY(*(cp+1), *(cp+2)), glyph); > > Need whitespace around '+'. > BTW, you can write them as cp[1] or cp[2]. No longer exist the code, but followed your suggestion in SurrogatePairAwareTextIterator::hasTrailingVariationSelector().
Created attachment 98649 [details] Patch V3
Updated the patch to fix incorrect logic in SurrogatePairAwareTextIterator::hasTrailingVariationSelector() (In reply to comment #15) > Created an attachment (id=98649) [details] > Patch V3
(Cc'ed author and reviewer of SurrogatePairAwareTextIterator class) Could someone review the latest patch?
I think that the names isUnicodeVariationSelector and isIdeographicVariationSelector are a bit misleading. The former matches the BMP variation selectors (code points <= U+FFFF), and the later matches the supplementary variation selectors (code points > U+FFFF). It is true that currently there is an equivalence between BMP/supplementary variation selectors and non-ideographic/ideographic variation sequences, but there is no reason that this could not change in the future. I would recommend to rename isUnicodeVariationSelector to isUnicodeBMPVariationSelector and to rename isIdeographicVariationSelector to isUnicodeSupplementaryVariationSelector. --- In LayoutTests/fast/text/unicode-variation-selector.html, why do you use scripting, and not just the static content '葛󠄀'?
Created attachment 114636 [details] Patch V4
Attachment 114636 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Last 3072 characters of output: ions.txt:3861: Path does not exist. fast/dom/Element/id-in-frame.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3862: Path does not exist. fast/frames/content-opacity-2.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3863: Path does not exist. fast/parser/close-while-stopping.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3864: Path does not exist. fast/frames/iframe-double-scale-contents.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3865: Path does not exist. fast/frames/set-parent-src-synchronously.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3868: Path does not exist. svg/custom/getBBox-path.svg [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3870: Path does not exist. svg/zoom/page/zoom-svg-as-relative-image.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3871: Path does not exist. svg/zoom/page/relative-sized-document-scrollbars.svg [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3873: Path does not exist. storage/indexeddb/key-type-array.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3875: Path does not exist. storage/indexeddb/factory-deletedatabase-interactions.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3877: Path does not exist. fast/js/array-functions-non-arrays.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3878: Path does not exist. fast/js/eval-cross-window.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3879: Path does not exist. fast/js/regexp-caching.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3880: Path does not exist. fast/js/toString-overrides.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3881: Path does not exist. fast/js/toString-recursion.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3882: Path does not exist. http/tests/security/xss-eval.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3884: Path does not exist. fast/dom/javascript-url-exception-isolation.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3885: Path does not exist. fast/borders/inline-mask-overlay-image-outset-vertical-rl.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3887: Path does not exist. fast/dom/rtl-scroll-to-leftmost-and-resize.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3889: Path does not exist. fast/canvas/canvas-transforms-fillRect-shadow.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3891: Path does not exist. fast/text/unicode-variation-selector.html [test/expectations] [2] Total errors found: 2140 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 114636 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=114636&action=review > LayoutTests/platform/chromium/test_expectations.txt:3898 > +BUGWK50999 : fast/text/unicode-variation-selector.html = MSSING FAIL Drive-by nit: MSSING -> MISSING
Comment on attachment 114636 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=114636&action=review > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:488 > + Vector<CGGlyph, 4> glyphs(4); This can just be an array. No reason to use a Vector.
Created attachment 114867 [details] Patch V5
Comment on attachment 114636 [details] Patch V4 View in context: https://bugs.webkit.org/attachment.cgi?id=114636&action=review Thank you for review. Revised the patch. >> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:488 >> + Vector<CGGlyph, 4> glyphs(4); > > This can just be an array. No reason to use a Vector. Done. >> LayoutTests/platform/chromium/test_expectations.txt:3898 >> +BUGWK50999 : fast/text/unicode-variation-selector.html = MSSING FAIL > > Drive-by nit: MSSING -> MISSING Done.
Attachment 114867 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Last 3072 characters of output: BBox-path.svg [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3854: Path does not exist. svg/zoom/page/zoom-svg-as-relative-image.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3855: Path does not exist. svg/zoom/page/relative-sized-document-scrollbars.svg [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3857: Path does not exist. storage/indexeddb/key-type-array.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3859: Path does not exist. storage/indexeddb/factory-deletedatabase-interactions.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3861: Path does not exist. fast/js/array-functions-non-arrays.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3862: Path does not exist. fast/js/eval-cross-window.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3863: Path does not exist. fast/js/regexp-caching.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3864: Path does not exist. fast/js/toString-overrides.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3865: Path does not exist. fast/js/toString-recursion.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3866: Path does not exist. http/tests/security/xss-eval.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3868: Path does not exist. fast/dom/javascript-url-exception-isolation.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3869: Path does not exist. fast/borders/inline-mask-overlay-image-outset-vertical-rl.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3871: Path does not exist. fast/dom/rtl-scroll-to-leftmost-and-resize.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3873: Path does not exist. http/tests/inspector/resource-tree/appcache-iframe-manifests.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3875: Path does not exist. accessibility/adjacent-continuations-cause-assertion-failure.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3877: Path does not exist. inspector/debugger/script-formatter.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3880: Path does not exist. fast/js/mozilla/strict/15.4.5.1.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3882: Path does not exist. fast/canvas/canvas-transforms-fillRect-shadow.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3884: Path does not exist. media/track/tracklist-is-reachable.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:3886: Path does not exist. fast/text/unicode-variation-selector.html [test/expectations] [2] Total errors found: 2135 in 18 files If any of these errors are false positives, please file a bug against check-webkit-style.
> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:489 > + CGGlyph glyphs[4]; > + wkGetGlyphsForCharacters(platformData().cgFont(), buffer, glyphs.data(), length); Now that "glyphs" is a CGGlyph array rather than a vector, I think you want to pass just "glyphs" to wkGetGlyphsForCharacters. > Source/WebCore/platform/graphics/win/SimpleFontDataWin.cpp:259 > + // FIXME: Implement. I think that the same implementation as for Mac should work here, except that the "system" function call, which would be: wkGetGlyphs(m_platformData.cgFont(), buffer, glyphs, length); However, I am not getting the answer I expect from that call; I not sure if that's because the call is incorrect, or the wkGetGlyphs simply does not support variation sequences. Do you (or somebody else) happen to know where the documentation could be, if not the implementation?
Created attachment 115753 [details] Patch
(In reply to comment #26) > Now that "glyphs" is a CGGlyph array rather than a vector, I think you want to pass just "glyphs" to wkGetGlyphsForCharacters. Oops, thank you for pointing this out. Fixed. > I think that the same implementation as for Mac should work here, except that the "system" function call, which would be: > > wkGetGlyphs(m_platformData.cgFont(), buffer, glyphs, length); > > However, I am not getting the answer I expect from that call; I not sure if that's because the call is incorrect, or the wkGetGlyphs simply does not support variation sequences. Do you (or somebody else) happen to know where the documentation could be, if not the implementation? Yes, it doesn't work as expected. That's why I didn't implement the function. I guess wkGetGlyphs() doesn't support UVS. It's behind the WebKitSupportLibrary which provided by Apple, and I don't think we can see the document or implementation about them. We might be able to implement this function by using Uniscribe.
There seems to be a bad interaction with selection. With the text "葛󠄀 葛󠄁 葛󠄂", if you start to select at the beginning and move the mouse towards the right, you will see that the area depicting the selection does not match the position of the mouse.
Created attachment 116176 [details] Patch
(In reply to comment #29) > There seems to be a bad interaction with selection. With the text "葛󠄀 葛󠄁 葛󠄂", if you start to select at the beginning and move the mouse towards the right, you will see that the area depicting the selection does not match the position of the mouse. Thank you for pointing this out. I misused m_lastCharacter in SurrogatePairAwareIterator::hasTrailingVariationSelector(). I should have used m_endCharacter instead.
Created attachment 118913 [details] Patch (rebased to ToT)
Comment on attachment 118913 [details] Patch (rebased to ToT) View in context: https://bugs.webkit.org/attachment.cgi?id=118913&action=review > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:470 > +void SimpleFontData::updateGlyphWithVariationSelector(UChar32 character, UChar32 selector, Glyph& glyph) const Hm, it should be possible to implement this in a cross platform fashion, no? I'd rather prefer a platform specific method that gives you a Glyph for a pair of UChar buffer and length. This way updateGlyphWithVariationSelector could live in SimpleFontData directly.
Comment on attachment 118913 [details] Patch (rebased to ToT) View in context: https://bugs.webkit.org/attachment.cgi?id=118913&action=review Hi Zimmermann, Thank you for review. >> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:470 >> +void SimpleFontData::updateGlyphWithVariationSelector(UChar32 character, UChar32 selector, Glyph& glyph) const > > Hm, it should be possible to implement this in a cross platform fashion, no? > I'd rather prefer a platform specific method that gives you a Glyph for a pair of UChar buffer and length. > This way updateGlyphWithVariationSelector could live in SimpleFontData directly. Some APIs take both charcode and variation selector in 32bits Unicode codepoint. For example, a Harfbuzz-ng's API that gives a glyph takes 32bit codepoints as arguments (hb_codepoint_t is a synonym for uint32_t): hb_bool_t hb_font_get_glyph (hb_font_t *font, hb_codepoint_t unicode, hb_codepoint_t variation_selector, hb_codepoint_t *glyph); So I think we should pass UChar32s to avoid duplicate conversion.
(In reply to comment #34) > (From update of attachment 118913 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=118913&action=review > > Hi Zimmermann, > > Thank you for review. You're welcome. > So I think we should pass UChar32s to avoid duplicate conversion. Oh yes, it was a typo, I meant to say UChar32.
(In reply to comment #35) > > So I think we should pass UChar32s to avoid duplicate conversion. > Oh yes, it was a typo, I meant to say UChar32. I think I misunderstand your point, but you mean I should change the function like below? updateGlyphWithVariationSelector(UChar32* characters, unsigned length, Glyph& glyph) The code in SimpleFontDataMac.mm converts utf32 to utf16 so changing the function like above doesn't contribute reducing the code. Or, you mean we should make the code live in SurrogatePairAwareTextIterator class?
(In reply to comment #36) > (In reply to comment #35) > > > So I think we should pass UChar32s to avoid duplicate conversion. > > Oh yes, it was a typo, I meant to say UChar32. > > I think I misunderstand your point, but you mean I should change the function like below? > > updateGlyphWithVariationSelector(UChar32* characters, unsigned length, Glyph& glyph) Okay, I'll resummarize: +void SimpleFontData::updateGlyphWithVariationSelector(UChar32 character, UChar32 selector, Glyph& glyph) const +{ + unsigned length = 0; + UChar buffer[4]; + + if (U_IS_BMP(character)) + buffer[length++] = character; + else { + buffer[length++] = U16_LEAD(character); + buffer[length++] = U16_TRAIL(character); + } + if (U_IS_BMP(selector)) + buffer[length++] = selector; + else { + buffer[length++] = U16_LEAD(selector); + buffer[length++] = U16_TRAIL(selector); + } My first thought was to move this into the cross-platform portion of SimpleFontData, aka. in platform/graphics/SimpleFontData.cpp. But if I understood you correctly, this conversion from UChar32 to UChar, and the following call to retrieve the replacment glyph, is specific to Mac? Aka. other platforms have APIs where you can pass in UChar32s directly. Is that correct? If so your approach, is probably right.
Sorry for confusion and thank you for summarization. (In reply to comment #37) > But if I understood you correctly, this conversion from UChar32 to UChar, and the following call to retrieve the replacment glyph, is specific to Mac? Aka. other platforms have APIs where you can pass in UChar32s directly. Is that correct? If so your approach, is probably right. Yes, this conversion is needed for wkGetGlyphsForCharacters() which is Mac specific API to get a glyph with variation selector. And some other platform can use APIs that take UChar32 directly(e.g. Harfbuzz-ng). I'm not sure which style (taking utf16 or utf32) is majority, though.
<rdar://problem/10572923>
Any other comments/suggestions? I'll re-upload the patch if there isn't.
Just go ahead, I'll review it :-)
Created attachment 119176 [details] Patch
(In reply to comment #41) > Just go ahead, I'll review it :-) Thanks:-) Uploaded.
Comment on attachment 119176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119176&action=review You're almost there, still r=me. I've missed some things in my last review, please fix those issues before landing. > Source/WebCore/ChangeLog:8 > + Adds SimpleFomtData::updateGlyphWithVariationSelector() which substitutes the glyph in question based on the selector. WidthIterator::advance() calls it when an unicode variation selector follows the character. Typo, SimpleFontData! :-) We also usually wrap lines here, eg. before 'glyph'. > Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:43 > + static const UChar trailStart = (0xDC00 | ((0xE0100 - 0x10000) & 0x3FF)); > + static const UChar trailEnd = (0xDC00 | ((0xE01EF - 0x10000) & 0x3FF)); This could make use of #define U16_TRAIL(supplementary) (UChar)(((supplementary)&0x3ff)|0xdc00). static const UChar trailStart = U16_TRAIL(0xE0100 - 0x10000); static const UChar trailEnd = U16_TRAIL(0xE01EF - 0x10000); Still magical, it deserves a comment, for those who don't know off-hand :-) > Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:44 > + return (lead == 0xDB40) && (trailStart <= trail && trail <= trailEnd); Ditto, a comment should be added regarding DB40. The braces are unnecessary as well. > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:470 > +void SimpleFontData::updateGlyphWithVariationSelector(UChar32 character, UChar32 selector, Glyph& glyph) const Hm, I didn't see it before, sorry, but you should avoid the code duplication: static inline void decomposeToUTF8(UChar* buffer, unsigned& length, const UChar& character) { if (U_IS_BMP(character)) { buffer[length] = character; ++length; return; } buffer[length] = U16_LEAD(character); buffer[length + 1] = U16_TRAIL(character); length += 2; } and then use it in updateGlyphWithVariationSelector: > Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:486 > + if (U_IS_BMP(character)) > + buffer[length++] = character; > + else { > + buffer[length++] = U16_LEAD(character); > + buffer[length++] = U16_TRAIL(character); > + } > + if (U_IS_BMP(selector)) > + buffer[length++] = selector; > + else { > + buffer[length++] = U16_LEAD(selector); > + buffer[length++] = U16_TRAIL(selector); > + } decomposeToUTF8(buffer, length, character); decomposeToUTF8(buffer + length, length, selector); ASSERT(length <= 4); That's it -- if you prefer to upload a final patch, feel free to.
Created attachment 119339 [details] Patch
Comment on attachment 119176 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119176&action=review Hi Nikolas, Thank you so much the review :-). I've addressed your comment and changed the code so I'd like to ask you to see the patch before I land it. Could you take another look? Thanks! >> Source/WebCore/ChangeLog:8 >> + Adds SimpleFomtData::updateGlyphWithVariationSelector() which substitutes the glyph in question based on the selector. WidthIterator::advance() calls it when an unicode variation selector follows the character. > > Typo, SimpleFontData! :-) We also usually wrap lines here, eg. before 'glyph'. Oops, thank you pointing this out. Done. >> Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:43 >> + static const UChar trailEnd = (0xDC00 | ((0xE01EF - 0x10000) & 0x3FF)); > > This could make use of #define U16_TRAIL(supplementary) (UChar)(((supplementary)&0x3ff)|0xdc00). > static const UChar trailStart = U16_TRAIL(0xE0100 - 0x10000); > static const UChar trailEnd = U16_TRAIL(0xE01EF - 0x10000); > Still magical, it deserves a comment, for those who don't know off-hand :-) Done. >> Source/WebCore/platform/graphics/SurrogatePairAwareTextIterator.cpp:44 >> + return (lead == 0xDB40) && (trailStart <= trail && trail <= trailEnd); > > Ditto, a comment should be added regarding DB40. The braces are unnecessary as well. Done. >> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:470 >> +void SimpleFontData::updateGlyphWithVariationSelector(UChar32 character, UChar32 selector, Glyph& glyph) const > > Hm, I didn't see it before, sorry, but you should avoid the code duplication: > > static inline void decomposeToUTF8(UChar* buffer, unsigned& length, const UChar& character) > { > if (U_IS_BMP(character)) { > buffer[length] = character; > ++length; > return; > } > > buffer[length] = U16_LEAD(character); > buffer[length + 1] = U16_TRAIL(character); > length += 2; > } > > and then use it in updateGlyphWithVariationSelector: Done. (Just change the name to decomposeToUTF16 and type of |character| to const UChar32). >> Source/WebCore/platform/graphics/mac/SimpleFontDataMac.mm:486 >> + } > > decomposeToUTF8(buffer, length, character); > decomposeToUTF8(buffer + length, length, selector); > ASSERT(length <= 4); > > That's it -- if you prefer to upload a final patch, feel free to. Changed as follows because the function uses length as index of the buffer: decomposeToUTF16(buffer, length, character); decomposeToUTF16(buffer, length, selector); ASSERT(length <= 4);
Comment on attachment 119339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119339&action=review Looks great, r=me. EWS doesn't like your patch, but it's the same as before modulo refactoring, so I think it's safe to land as-is. Thanks for correcting my incorrect ToUTF8 method ;-) > LayoutTests/ChangeLog:9 > + A tiny font (gw432047.ttf), which comes from glyphwiki.org and is used by Moziila, is added for the test. typo: Mozilla. > LayoutTests/ChangeLog:14 > + * platform/chromium/test_expectations.txt: Added fast/text/unicode-variation-selector.html. This test need rebaseline. Just noticed that you're missing a mac result. Do you plan to land the new results, once you got results from the mac bots?
Created attachment 119398 [details] Patch for landing
Comment on attachment 119339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=119339&action=review Thank you for r+ :-) Addressed your comments. >> LayoutTests/ChangeLog:9 >> + A tiny font (gw432047.ttf), which comes from glyphwiki.org and is used by Moziila, is added for the test. > > typo: Mozilla. Once again, thank you for pointing typo. Fixed. >> LayoutTests/ChangeLog:14 >> + * platform/chromium/test_expectations.txt: Added fast/text/unicode-variation-selector.html. This test need rebaseline. > > Just noticed that you're missing a mac result. Do you plan to land the new results, once you got results from the mac bots? Yes. I'll add the result and update expectations after bot create the results.
Comment on attachment 119398 [details] Patch for landing Clearing flags on attachment: 119398 Committed r102915: <http://trac.webkit.org/changeset/102915>
All reviewed patches have been landed. Closing bug.