[webkit-reviews] review granted: [Bug 50999] Supports Unicode variation selector : [Attachment 119176] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has granted Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 50999: Supports Unicode variation selector
https://bugs.webkit.org/show_bug.cgi?id=50999

Attachment 119176: Patch
https://bugs.webkit.org/attachment.cgi?id=119176&action=review

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
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.