Bug 50999

Summary: Supports Unicode variation selector
Product: WebKit Reporter: Kenichi Ishibashi <bashi>
Component: TextAssignee: Kenichi Ishibashi <bashi>
Status: RESOLVED FIXED    
Severity: Enhancement CC: bashi, cc-bugs, dglazkov, emuller, hamaji, hayato, jamesr, jshin, krit, mitz, VYV03354, webkit-bug-importer, webkit.review.bot, yuzo, zimmermann
Priority: P3 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch V0
none
Patch V1
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch V2
none
Patch V3
none
Patch V4
none
Patch V5
none
Patch
none
Patch
none
Patch (rebased to ToT)
none
Patch
none
Patch
none
Patch for landing none

Description Kenichi Ishibashi 2010-12-13 18:32:53 PST
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.
Comment 1 Kenichi Ishibashi 2010-12-16 21:31:57 PST
Created attachment 76844 [details]
Patch V0
Comment 2 Kenichi Ishibashi 2010-12-16 21:34:16 PST
(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).
Comment 3 Kenichi Ishibashi 2010-12-16 21:36:01 PST
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 4 Yuzo Fujishima 2011-01-04 19:45:35 PST
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.
Comment 5 Kenichi Ishibashi 2011-01-31 16:56:48 PST
Hi,

Could anyone could review this?
Comment 6 Kenichi Ishibashi 2011-02-13 18:33:21 PST
Ping?

(In reply to comment #5)
> Hi,
> 
> Could anyone could review this?
Comment 7 Kent Tamura 2011-04-26 18:09:17 PDT
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
Comment 8 Kenichi Ishibashi 2011-06-24 03:52:55 PDT
Created attachment 98478 [details]
Patch V1
Comment 9 Kenichi Ishibashi 2011-06-24 03:56:38 PDT
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 10 Kent Tamura 2011-06-24 04:14:42 PDT
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 11 WebKit Review Bot 2011-06-24 04:25:58 PDT
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
Comment 12 WebKit Review Bot 2011-06-24 04:26:03 PDT
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
Comment 13 Kenichi Ishibashi 2011-06-25 03:02:57 PDT
Created attachment 98585 [details]
Patch V2
Comment 14 Kenichi Ishibashi 2011-06-25 03:05:48 PDT
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().
Comment 15 Kenichi Ishibashi 2011-06-26 17:59:26 PDT
Created attachment 98649 [details]
Patch V3
Comment 16 Kenichi Ishibashi 2011-06-26 18:03:09 PDT
Updated the patch to fix incorrect logic in SurrogatePairAwareTextIterator::hasTrailingVariationSelector()

(In reply to comment #15)
> Created an attachment (id=98649) [details]
> Patch V3
Comment 17 Kenichi Ishibashi 2011-07-13 18:35:23 PDT
(Cc'ed author and reviewer of SurrogatePairAwareTextIterator class)

Could someone review the latest patch?
Comment 18 Eric Muller 2011-11-10 13:12:50 PST
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;'?
Comment 19 Kenichi Ishibashi 2011-11-11 00:09:57 PST
Created attachment 114636 [details]
Patch V4
Comment 20 WebKit Review Bot 2011-11-11 00:14:48 PST
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 21 Peter Beverloo 2011-11-11 04:10:15 PST
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 22 Darin Adler 2011-11-11 14:27:00 PST
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.
Comment 23 Kenichi Ishibashi 2011-11-13 16:08:15 PST
Created attachment 114867 [details]
Patch V5
Comment 24 Kenichi Ishibashi 2011-11-13 16:11:03 PST
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.
Comment 25 WebKit Review Bot 2011-11-13 16:11:46 PST
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.
Comment 26 Eric Muller 2011-11-17 18:33:32 PST
> 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?
Comment 27 Kenichi Ishibashi 2011-11-18 00:19:32 PST
Created attachment 115753 [details]
Patch
Comment 28 Kenichi Ishibashi 2011-11-18 00:21:22 PST
(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.
Comment 29 Eric Muller 2011-11-18 09:28:17 PST
There seems to be a bad interaction with selection. With the text "&#x845b;&#xE0100; &#x845b;&#xe0101; &#x845b;&#xe0102;", 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.
Comment 30 Kenichi Ishibashi 2011-11-21 21:19:34 PST
Created attachment 116176 [details]
Patch
Comment 31 Kenichi Ishibashi 2011-11-21 21:23:03 PST
(In reply to comment #29)
> There seems to be a bad interaction with selection. With the text "&#x845b;&#xE0100; &#x845b;&#xe0101; &#x845b;&#xe0102;", 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.
Comment 32 Kenichi Ishibashi 2011-12-12 17:07:12 PST
Created attachment 118913 [details]
Patch (rebased to ToT)
Comment 33 Nikolas Zimmermann 2011-12-13 00:34:48 PST
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 34 Kenichi Ishibashi 2011-12-13 01:01:50 PST
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.
Comment 35 Nikolas Zimmermann 2011-12-13 01:11:49 PST
(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.
Comment 36 Kenichi Ishibashi 2011-12-13 01:30:18 PST
(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?
Comment 37 Nikolas Zimmermann 2011-12-13 01:57:38 PST
(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.
Comment 38 Kenichi Ishibashi 2011-12-13 02:24:52 PST
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.
Comment 39 Radar WebKit Bug Importer 2011-12-13 09:41:25 PST
<rdar://problem/10572923>
Comment 40 Kenichi Ishibashi 2011-12-13 16:29:45 PST
Any other comments/suggestions? I'll re-upload the patch if there isn't.
Comment 41 Nikolas Zimmermann 2011-12-14 01:01:18 PST
Just go ahead, I'll review it :-)
Comment 42 Kenichi Ishibashi 2011-12-14 01:14:36 PST
Created attachment 119176 [details]
Patch
Comment 43 Kenichi Ishibashi 2011-12-14 01:15:46 PST
(In reply to comment #41)
> Just go ahead, I'll review it :-)

Thanks:-) Uploaded.
Comment 44 Nikolas Zimmermann 2011-12-14 01:37:39 PST
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.
Comment 45 Kenichi Ishibashi 2011-12-14 17:04:20 PST
Created attachment 119339 [details]
Patch
Comment 46 Kenichi Ishibashi 2011-12-14 17:08:38 PST
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 47 Nikolas Zimmermann 2011-12-15 01:35:00 PST
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?
Comment 48 Kenichi Ishibashi 2011-12-15 01:43:15 PST
Created attachment 119398 [details]
Patch for landing
Comment 49 Kenichi Ishibashi 2011-12-15 01:46:24 PST
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 50 WebKit Review Bot 2011-12-15 03:57:22 PST
Comment on attachment 119398 [details]
Patch for landing

Clearing flags on attachment: 119398

Committed r102915: <http://trac.webkit.org/changeset/102915>
Comment 51 WebKit Review Bot 2011-12-15 03:57:32 PST
All reviewed patches have been landed.  Closing bug.