[webkit-reviews] review granted: [Bug 205990] A partially selected RTL text is placed at a wrong vertical position if it has a vertical initial advance : [Attachment 388209] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Jan 20 10:25:14 PST 2020


Darin Adler <darin at apple.com> has granted Fujii Hironori
<Hironori.Fujii at sony.com>'s request for review:
Bug 205990: A partially selected RTL text is placed at a wrong vertical
position if it has a vertical initial advance
https://bugs.webkit.org/show_bug.cgi?id=205990

Attachment 388209: Patch

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




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 388209
  --> https://bugs.webkit.org/attachment.cgi?id=388209
Patch

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

Looks like the Windows build failed — I couldn't find the error message in the
EWS results, though. I retried so maybe it will succeed without any further
changes.

> Source/WebCore/platform/graphics/ComplexTextController.h:68
> +    float totalWidth() const { return m_totalAdvance.width(); }

Are there any uses of this outside the class? Can this be a private member
function instead? Or maybe we can just write m_totalAdvance.width() at each of
the two call sites. I’d like to cut down on proliferation of functions that do
nearly the same thing.

> Source/WebCore/platform/graphics/FontCascade.cpp:292
> +    FloatPoint oldStartPoint(startPoint);

I suggest that instead of this, we write:

    float oldStartX = startPoint.x();

> Source/WebCore/platform/graphics/FontCascade.cpp:1438
> +    FloatPoint startPoint(point);

Style wise, I think this is nicer with an "=", but not sure everyone else will
agree.

> Source/WebCore/platform/graphics/GlyphBuffer.h:66
> +    operator FloatSize() const { return FloatSize(width(), height()); }

I think it’s a little risky to add an operator that does a lossy conversion
from doubles to floats, implicitly and silently. Normally we would like
conversions that narrow to be named function calls.


More information about the webkit-reviews mailing list