[webkit-reviews] review granted: [Bug 213446] Text manipulation should exclude text rendered using icon-only fonts : [Attachment 402486] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 213446: Text manipulation should exclude text rendered using icon-only
fonts
https://bugs.webkit.org/show_bug.cgi?id=213446

Attachment 402486: Patch

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




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

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

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:662
> +bool Font::shouldExcludeFontFromTextManipulation() const

I'm not sure this actually belongs in Font. Font is a platform/ class, which
indicates (at least to me) that all its functionality should be generally
applicable to the various clients in WebCore/WebKit who want to use it.

Historically, I'm really bad at answering the question "should this function
belong in one place or another" so if you feel that it really belongs here, I'm
probably wrong.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:664
> +    if (origin() != Origin::Remote)

Does translation work in WKWebView? (WKWebView doesn't disallow user-installed
fonts by default.) We may want this logic to be insensitive to web fonts vs
installed fonts. Or maybe not.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:667
> +    auto platformFont = platformData().ctFont();

I think you want .font() instead here

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:671
> +    auto supportedCharacters =
adoptCF(CTFontCopyCharacterSet(platformFont));

Is this call expensive? It might be cheaper to call
CTFontGetGlyphsForCharacters("a") for the test on line 676.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:672
> +    if (CFCharacterSetHasMemberInPlane(supportedCharacters.get(), 1) ||
CFCharacterSetHasMemberInPlane(supportedCharacters.get(), 2))

I assume there's no reason for this other than "it's a heuristic"?

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:678
> +	   bool foundGlyph = CTFontGetGlyphsForCharacterRange(platformFont,
&lowercaseAGlyph, CFRangeMake('a', 1));

Is it worth making a range instead of CTFontGetGlyphsForCharacters here?

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:683
> +    constexpr UniChar firstCharacterToTest = ' ';

Might want to add a comment saying "this includes the uppercase and lowercase
alphabet" or whatever. I haven't memorized the ascii table.

Do we really need to test almost a hundred characters? Getting bounding boxes
isn't free.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:687
> +    CGGlyph glyphs[numberOfCharactersToTest];

Is this too much stack space? I don't know

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:690
> +    CGRect boundingRects[numberOfCharactersToTest];

Ditto

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:691
> +    CTFontGetBoundingRectsForGlyphs(platformFont, kCTFontOrientationDefault,
glyphs, boundingRects, numberOfCharactersToTest);

Might want to consider de-duping duplicate glyphs (where multiple characters
map to the same glyph). It might be worth it.

I'd expect such duplication would only be common when multiple characters map
to glyphID 0.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:694
> +	   if (!CFCharacterSetIsCharacterMember(supportedCharacters.get(),
character))

You can get this information faster if you just compare glyphs[character -
firstCharacterToTest] to 0

> Source/WebCore/platform/graphics/cocoa/FontPlatformDataCocoa.mm:186
> +    if (auto platformFont = font())

Consider:

@font-face {
    font-family: FontA;
    src: url("FontB.ttf");
}
...
font-family: FontA, MyFallbackFont;

How confident are you that this comparison should be done on FontA?

Similarly, what if the user has content blockers, and FontA has not been
downloaded yet? Or the site loads fonts themselves in JS and we trigger text
manipulation before the JS is done? In this situation, primaryFont() will
return MyFallbackFont instead of FontA. But the content still says WORLD or
whatever.

I didn't see anything in the ChangeLog about this.