[webkit-reviews] review denied: [Bug 157629] Implement more of the canvas TextMetrics API : [Attachment 349684] Patch

[bugzilla-daemon at webkit.org]

Myles C. Maxfield <mmaxfield at apple.com> has denied Justin Michaud
<justin_michaud at apple.com>'s request for review:
Bug 157629: Implement more of the canvas TextMetrics API
https://bugs.webkit.org/show_bug.cgi?id=157629

Attachment 349684: Patch

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




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

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

I'm not sure this API is mature enough to implement. I don't quite understand
how other browsers have implemented it given how many standardization
discussions we're having about it.

> Source/WebCore/ChangeLog:12
> +	   No new tests (OOPS!).

Please update this to say the name of the test that you updated.

> Source/WebCore/platform/graphics/FontCascade.cpp:294
> +    advances.append(glyphBuffer.initialAdvance().width());

The Web API right now should probably ignore the initial advance.
https://github.com/whatwg/html/issues/4030#issuecomment-420945138

> Source/WebCore/platform/graphics/FontCascade.cpp:329
> +    while (nextGlyph < glyphBuffer.size()) {
> +	   const Font* nextFontData = glyphBuffer.fontAt(nextGlyph);
> +
> +	   if (nextFontData != fontData) {
> +	       fontData = nextFontData;
> +	       currentOffset = lastOffset + 1;
> +	   }
> +
> +	   lastOffset = glyphBuffer.offsetInString(nextGlyph) + currentOffset;
> +	   offsets.append(lastOffset);
> +	   nextGlyph++;
> +    }

We have logic in ComplexTextController::offsetForPosition() that does this. We
should investigate pulling that code out into a shared function that we can
call here instead.

Specifically, look for // FIXME: Instead of dividing the glyph's advance
equally between the characters, this