[webkit-reviews] review granted: [Bug 187209] [Cocoa] LastResort in the font family list causes emoji with joiners to be rendered as multiple .notdef characters : [Attachment 343984] 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 187209: [Cocoa] LastResort in the font family list causes emoji with
joiners to be rendered as multiple .notdef characters
https://bugs.webkit.org/show_bug.cgi?id=187209

Attachment 343984: Patch

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




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

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

> Source/WebCore/platform/graphics/Font.cpp:158
> +    // FIXME: Consider reordering these so the most common ones are at the
front.
> +    // Doing this could cause the BitVector to fit inside inline storage and
therefore
> +    // be both a performance and a memory progression.

What if there’s a typo in the numbers or a logic mistake in this function where
we accidentally used a number twice? How would we notice? What test would
detect the mistake?

> Source/WebCore/platform/graphics/Font.cpp:585
> +    // This is very similar to
static_cast<bool>(glyphForCharacter(character))
> +    // except that glyphForCharacter() maps certain code points to ZWS
(because they
> +    // shouldn't show up in the fast text codepath). This function doesn't
do that
> +    // mapping, and instead is as honest as possible about what code points
the font
> +    // supports.

The glyphForCharacter function maps certain code points to zero-width spaces
because we never want those code points to render visibly. We want that
behavior for both the fast and slow text code paths. I wish the comment was
clearer on this point. And I wish I understood how we will get that behavior.

The comment does not make clear why "as honest as possible" is needed here. I
think the idea is that "even if we don’t want the code points to render visibly
we want to correctly understood the code points are 'supported' because we
makes some other kind of decision based on that". But I don’t fully understand.

The comment also doesn’t make it clear why we decided that this can be the
slower function and the other one must be the faster function, rather than the
other way around. Seems natural to make the "honest" function the fast one, but
I’m sure we have a good reason not to do that.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:612
> +    CFIndex count;
> +    if (U_IS_BMP(character)) {
> +	   codeUnits[0] = character;
> +	   count = 1;
> +    } else {
> +	   codeUnits[0] = U16_LEAD(character);
> +	   codeUnits[1] = U16_TRAIL(character);
> +	   count = 2;
> +    }

This code can be written like this:

    CFIndex count = 0;
    U16_APPEND_UNSAFE(codeUnits, count, character);

I think it’s better that way.