[webkit-reviews] review granted: [Bug 239484] [GPU Process] Vertical text is incorrectly displaced : [Attachment 458034] Patch

[bugzilla-daemon at webkit.org]

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

Attachment 458034: Patch

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




--- Comment #6 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 458034
  --> https://bugs.webkit.org/attachment.cgi?id=458034
Patch

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

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:218
> +static Vector<CGSize> computeHorizontalAdvancesFromPositions(const CGPoint
positions[], size_t count, const CGAffineTransform& textMatrix)

I think this function should return AdvancesAndInitialPosition also. The
initialPenPosition is calculated in DrawGlyphsRecorder::recordDrawGlyphs() for
the horizontal case like this:

    initialPenPosition = textMatrix.mapPoint(positions[0]);

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:223
>      Vector<CGSize> result;
> +    result.reserveInitialCapacity(count);

Can't this written like the one below:

   Vector<CGSize, 256> result(count);

I honestly do not understand the difference between setting the initialCapcity
of the Vector, passing the size in the constructor and calling
reserveInitialCapacity()? What is the most efficient way to create a fixed
length Vector?

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:230
> +    result.uncheckedConstructAndAppend(CGSizeMake(0, 0));

Should this append CGSizeZero like what computeVerticalAdvancesFromPositions
does?

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:234
> +struct AdvancesAndInitialPosition {

Should this struct have a constructor which takes the size of the advances and
initializes its capacity?

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:244
> +    if (!count)
> +	   return { { }, CGPointZero };

This special case is already handled by the caller
DrawGlyphsRecorder::recordDrawGlyphs()? I do not think we should be drawing
anything if count is zero. So this function should not be called in this case.
I think we can add an ASSERT(count) here and in
computeHorizontalAdvancesFromPositions() as well.

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:251
> +	   auto translationInUserCoordinates =
CGSizeApplyAffineTransform(translation, syntheticObliqueLessTextMatrix);
> +	   return CGPointMake(positionInUserCoordinates.x -
translationInUserCoordinates.width, positionInUserCoordinates.y -
translationInUserCoordinates.height);

I am not sure if this would be clear/clean or not, but are these statements
equivalent to?

    return CGPointMake(positionInUserCoordinates.x + translation.height,
positionInUserCoordinates.y + translation.width);

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:262
> +	  
result.advances.uncheckedConstructAndAppend(CGSizeMake(nextPosition.x -
previousPosition.x, nextPosition.y - previousPosition.y));

I find It diffract to read this code which says "currentAdvance = nextPosition
- previousPosition". The question is: what is the current position then?

Since I starts with 1, I would suggest using "currentPosition" instead of
nextPosition.

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:319
> +    FloatPoint initialPenPosition;
>  
> +    Vector<CGSize> advances;

These can replaced by the following statement if
computeHorizontalAdvancesFromPositions returns AdvancesAndInitialPosition:

    AdvancesAndInitialPosition result;

> Source/WebCore/platform/graphics/coretext/DrawGlyphsRecorderCoreText.cpp:321
> +	   Vector<CGSize, 256> translations(count);

Is there a reason for setting the initialCapacity and the size of a fixed-size
vector?

> Source/WebCore/platform/graphics/coretext/FontCascadeCoreText.cpp:266
> +    auto syntheticObliqueLessTextMatrix =
computeBaseVerticalTextMatrix(computeBaseOverallTextMatrix(std::nullopt));

I am confused by the name and the calculation of this matrix.

computeBaseOverallTextMatrix(std::nullopt) returns AffineTransform().flipyY() =
AffineTransform(1, 0, 0, -1, 0, 0)
computeBaseVerticalTextMatrix(computeBaseOverallTextMatrix(std::nullopt))
returns rotateLeftTransform() * computeBaseOverallTextMatrix(std::nullopt) =
AffineTransform(0, -1, 1, 0, 0, 0) * AffineTransform(1, 0, 0, -1, 0, 0)

So everything here is constant. Can't this be a "static constexpr"? Can't we
choose a better name which reflects it is constant matrix?