[webkit-reviews] review granted: [Bug 215059] [Cocoa] Migrate from CTFontTransformGlyphsWithLanguage() to CTFontShapeGlyphs() : [Attachment 406135] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 215059: [Cocoa] Migrate from CTFontTransformGlyphsWithLanguage() to
CTFontShapeGlyphs()
https://bugs.webkit.org/show_bug.cgi?id=215059

Attachment 406135: Patch

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




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

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

> Source/WebCore/platform/graphics/FontCascade.h:119
> +    float widthForSimpleText(StringView text, TextDirection =
TextDirection::LTR /* matches default argument of TextRun constructor */)
const;

Why does it matter that these defaults match?

> Source/WebCore/platform/graphics/TextRun.h:46
> +    // LTR default argument matches FontCascade::widthForSimpleText()'s
default argument

Need period at end of comment.

Why does it matter that these defaults match?

> Source/WebCore/platform/graphics/TextRun.h:63
> +    // LTR default argument matches FontCascade::widthForSimpleText()'s
default argument

Need period at end of comment.

Why does it matter that these defaults match?

> Source/WebCore/platform/graphics/WidthIterator.cpp:116
> +    // CTFontTransformGlyphs() operates in visual order, but WidthIterator
iterates in logical order.
> +    // Temporarily put us in visual order just for the call, then put us
back into logical order when
> +    // the call is done.
> +    // We don't have a global view of the entire GlyphBuffer; we're just
operating on a single chunk of it.
> +    // WidthIterator encounters the chunks out in logical order, so we have
to maintain that invariant.
> +    // Eventually, FontCascade::layoutSimpleText() will reverse the whole
buffer to put the entire thing
> +    // in visual order, but that's okay because it has a view of the entire
GlyphBuffer.
> +    // On the other hand, CTFontShapeGlyphs() accepts the buffer in logical
order but returns it in physical
> +    // order, which means the second reverse() in this function still needs
to execute when
> +    // CTFontShapeGlyphs() is being used.
> +#if !USE(CTFONTSHAPEGLYPHS)

This is strange. It’s cross platform code, but the comment seems to imply that
all platforms use either CTFontTransformGlyphs or CTFontShapeGlyphs. Can we
create some kind of cross-platform abstraction here? Typically the
cross-platform code doesn’t need long comments about the specific
platform-dependent functions used. In modern code could even do something like:

    if constexpr (Font::applyTransformsTakesGlyphsInVisualOrder)

Maybe it’s "CTFontTransformGlyphs and all the other functions used for this
purpose on all the other platforms?"

> LayoutTests/ChangeLog:13
> +	   * fast/text/soft-hyphen-2-expected.txt: Removed. Platform-specific
rendertree dumps
> +	   need to be in platform/ subfolders.
> +	   * fast/text/soft-hyphen-3-expected.txt: Removed.
> +	   * fast/text/soft-hyphen-4-expected.txt: Removed.

Can any of these tests be changed into ref tests?