[webkit-reviews] review granted: [Bug 232917] [GPU Process] [iOS] Vertical text is incorrectly displaced : [Attachment 456209] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Said Abou-Hallawa
<sabouhallawa at apple.com>'s request for review:
Bug 232917: [GPU Process] [iOS] Vertical text is incorrectly displaced
https://bugs.webkit.org/show_bug.cgi?id=232917

Attachment 456209: Patch

https://bugs.webkit.org/attachment.cgi?id=456209&action=review




--- Comment #9 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 456209
  --> https://bugs.webkit.org/attachment.cgi?id=456209
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=456209&action=review

r=me because this is a step in the right direction, but I think there's more
work to make this function exactly equal to the inverse of
fillVectorWithVerticalGlyphPositions().

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:259
> +    auto position = textMatrix.mapPoint(positions[0]);

nit: a better name could be "initialPenPosition" or something

>>
Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:-260
>> -	    // Keep this in sync as the inverse of
`fillVectorWithVerticalGlyphPositions`.
> 
> I think I should keep this comment. The new code below does exactly what it
says.

Agreed. It's important to keep the reference to
fillVectorWithVerticalGlyphPositions().

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:271
> +    m_owner.drawGlyphsAndCacheFont(font, glyphs,
computeAdvancesFromPositions(positions, count, textMatrix).data(), count,
position, m_smoothingMode);

This doesn't use rotateLeftTransform(). fillVectorWithVerticalGlyphPositions()
does use rotateLeftTransform(), and this is supposed to be the inverse of that
function. So I think there's more work that needs to be done here.