Bug 50999 – Supports Unicode variation selector

• Component: Text
• Status: RESOLVED FIXED

[Kenichi Ishibashi]

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.

[Kenichi Ishibashi]

Created attachment 76844 [details]
Patch V0

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

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)

[Yuzo Fujishima]

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.

[Kenichi Ishibashi]

Hi,

Could anyone could review this?

[Kenichi Ishibashi]

Ping?

(In reply to comment #5)
> Hi,
> 
> Could anyone could review this?

[Kent Tamura]

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

[Kenichi Ishibashi]

Created attachment 98478 [details]
Patch V1

[Kenichi Ishibashi]

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

[Kent Tamura]

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].

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kenichi Ishibashi]

Created attachment 98585 [details]
Patch V2

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

Created attachment 98649 [details]
Patch V3

[Kenichi Ishibashi]

Updated the patch to fix incorrect logic in SurrogatePairAwareTextIterator::hasTrailingVariationSelector()

(In reply to comment #15)
> Created an attachment (id=98649) [details]
> Patch V3

[Kenichi Ishibashi]

(Cc'ed author and reviewer of SurrogatePairAwareTextIterator class)

Could someone review the latest patch?

[Eric Muller]

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 '&#x845b;&#xE0100;'?

[Kenichi Ishibashi]

Created attachment 114636 [details]
Patch V4

[WebKit Review Bot]

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.

[Peter Beverloo]

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

[Darin Adler]

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.

[Kenichi Ishibashi]

Created attachment 114867 [details]
Patch V5

[Kenichi Ishibashi]

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.

[WebKit Review Bot]

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.

[Eric Muller]

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

[Kenichi Ishibashi]

Created attachment 115753 [details]
Patch

[Kenichi Ishibashi]

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

[Eric Muller]

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.

[Kenichi Ishibashi]

Created attachment 116176 [details]
Patch

[Kenichi Ishibashi]

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

[Kenichi Ishibashi]

Created attachment 118913 [details]
Patch (rebased to ToT)

[Nikolas Zimmermann]

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.

[Kenichi Ishibashi]

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.

[Nikolas Zimmermann]

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

[Kenichi Ishibashi]

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

[Nikolas Zimmermann]

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

[Kenichi Ishibashi]

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.

[Radar WebKit Bug Importer]

<rdar://problem/10572923>

[Kenichi Ishibashi]

Any other comments/suggestions? I'll re-upload the patch if there isn't.

[Nikolas Zimmermann]

Just go ahead, I'll review it :-)

[Kenichi Ishibashi]

Created attachment 119176 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #41)
> Just go ahead, I'll review it :-)

Thanks:-) Uploaded.

[Nikolas Zimmermann]

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.

[Kenichi Ishibashi]

Created attachment 119339 [details]
Patch

[Kenichi Ishibashi]

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

[Nikolas Zimmermann]

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?

[Kenichi Ishibashi]

Created attachment 119398 [details]
Patch for landing

[Kenichi Ishibashi]

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.

[WebKit Review Bot]

Comment on attachment 119398 [details]
Patch for landing

Clearing flags on attachment: 119398

Committed r102915: <http://trac.webkit.org/changeset/102915>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.