[webkit-reviews] review granted: [Bug 235053] REGRESSION(r281389): using font-variant-ligatures causes Unicode bidi isolation control characters to render : [Attachment 449318] 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 235053: REGRESSION(r281389): using font-variant-ligatures causes Unicode
bidi isolation control characters to render
https://bugs.webkit.org/show_bug.cgi?id=235053

Attachment 449318: Patch

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




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

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

> Source/WebCore/platform/graphics/ComplexTextController.cpp:834
> +	   if (!u_hasBinaryProperty(character,
UCHAR_DEFAULT_IGNORABLE_CODE_POINT) && character != objectReplacementCharacter)

Is this efficient enough as-is, or do we need a fast path that skips the call
to u_hasBinaryProperty for the most common characters? I suppose we did a
memory allocation here so it’s not super-performance-sensitive.

> Source/WebCore/platform/graphics/WidthIterator.cpp:622
> +	       || (characterResponsibleForThisGlyph >= deleteCharacter &&
characterResponsibleForThisGlyph < noBreakSpace)
> +	       || characterResponsibleForThisGlyph ==
objectReplacementCharacter
> +	       || u_hasBinaryProperty(characterResponsibleForThisGlyph,
UCHAR_DEFAULT_IGNORABLE_CODE_POINT)) {

I don’t understand the [nullCharacter,space) and [delete,noBreakSpace) parts of
this expression; is that an optimization or are those not ignorable? Can we
make this clearer with a helper function? If it’s just an optimization that
could go into the helper function. Since this repeats the
UCHAR_DEFAULT_IGNORABLE_CODE_POINT + objectReplacementCharacter logic in
ComplexTextController.cpp, a helper would make both call sites less wordy and
give us a place to write a comment or use a name that makes it clear why we
need that exception. Even if those other checks are not just an optimization.