[webkit-reviews] review granted: [Bug 77568] [Cocoa] Glyph lookup should be language-sensitive (specifically between Yiddish and Hebrew) : [Attachment 387487] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 77568: [Cocoa] Glyph lookup should be language-sensitive (specifically
between Yiddish and Hebrew)
https://bugs.webkit.org/show_bug.cgi?id=77568

Attachment 387487: Patch

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




--- Comment #64 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 387487
  --> https://bugs.webkit.org/attachment.cgi?id=387487
Patch

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

> Source/WebCore/platform/graphics/GlyphBuffer.h:172
> +	   for (size_t i = 0; i < length; ++i)
> +	       m_fonts[location + i] = font;

Maybe i = location; I < location + length, and you could precompute location +
length as end.

Also originalSize - location is computed multiple times.

> Source/WebCore/platform/graphics/WidthIterator.cpp:126
> +    // This is totally, 100%, furiously, utterly, frustratingly bogus.
> +    // There is absolutely no guarantee that glyph indices before shaping
have any relation at all with glyph indices after shaping.

Perhaps turn this anguished comment into an actionable bug and reference it
here.

> Source/WebCore/platform/graphics/cocoa/FontCocoa.mm:562
> +    auto handler = ^(CFRange range, CGGlyph** newGlyphsPointer, CGSize**
newAdvancesPointer) {
> +	   range.location = std::min(std::max(range.location,
static_cast<CFIndex>(0)), static_cast<CFIndex>(glyphBuffer.size()));
> +	   if (range.length < 0) {
> +	       range.length = std::min(range.location, -range.length);
> +	       range.location = range.location - range.length;
> +	       glyphBuffer.remove(beginningIndex + range.location,
range.length);
> +	   } else
> +	       glyphBuffer.makeHole(beginningIndex + range.location,
range.length, this);
> +	   *newGlyphsPointer = glyphBuffer.glyphs(beginningIndex);
> +	   *newAdvancesPointer = glyphBuffer.advances(beginningIndex);
> +    };
> +    CTFontTransformGlyphsWithLanguage(m_platformData.ctFont(),
glyphBuffer.glyphs(beginningIndex),
reinterpret_cast<CGSize*>(glyphBuffer.advances(beginningIndex)),
glyphBuffer.size() - beginningIndex, options,
platformData().locale().string().createCFString().get(), handler);

It seems that this could fall into pathological O(n^2) cases where makeHole or
glyphBuffer.remove are called over and over and each time move the rest of the
buffers. Is there a more efficient way?