Bug 234451 – Use character names instead of hex codes in FontCascade.h

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

Use character names instead of hex codes in FontCascade.h

[Myles C. Maxfield]

Created attachment 447489 [details]
Patch

[EWS]

Committed r287250 (245408@main): <https://commits.webkit.org/245408@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 447489 [details].

[Radar WebKit Bug Importer]

<rdar://problem/86703692>

[Darin Adler]

Comment on attachment 447489 [details]
Patch

I agree that these are easier to understand when it's ==.

But when using <= and >= I am not so sure.

[Myles C. Maxfield]

I think it could go either way. I did them here for consistency, but I’m happy to put them back if people (or you) think it’s better.

[Darin Adler]

May seem like overkill, but I would have made named functions for those sets of characters rather than using <= >= inline in that function. Might even put those named functions in CharacterNames.h.

[Darin Adler]

Comment on attachment 447489 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCascade.h:264
> +    static bool treatAsZeroWidthSpaceInComplexScript(UChar32 c) { return c < space || (c >= deleteCharacter && c < noBreakSpace) || c == softHyphen || c == zeroWidthSpace || (c >= leftToRightMark && c <= rightToLeftMark) || (c >= leftToRightEmbed && c <= rightToLeftOverride) || c == zeroWidthNoBreakSpace || c == objectReplacementCharacter; }

It seems like we should change these:

    c < space
        -> isC0ControlCharacter(c)
    (c >= leftToRightMark && c <= rightToLeftMark) || (c >= leftToRightEmbed && c <= rightToLeftOverride)
        -> isDirectionalFormattingCharacter(c)

But to deserve the name isDirectionalFormattingCharacter, it would include 5 other characters <https://www.unicode.org/reports/tr9/#Directional_Formatting_Characters>. It seems clear that those 5 other characters also should also be treated as zero-width space. Unless maybe it’s not so important to have that be an exhaustive list. Unclear what the importance of including characters here is, and how we test it. Presumably it works around incorrectly constructed fonts where those characters aren’t zero-width?

If we were not using an isDirectionalFormattingCharacter function, then we'd want to do this:

    (c >= leftToRightMark && c <= rightToLeftMark)
        -> c == leftToRightMark || c == rightToLeftMark

But more importantly, what character are covered in the range leftToRightEmbed through rightToLeftOverride is not self-explanatory.