[webkit-reviews] review granted: [Bug 49353] ComplexTextController not prepared to handle multiple runs : [Attachment 73555] Proposed changes.

[bugzilla-daemon at webkit.org]

mitz at webkit.org has granted nholbrook at apple.com's request for review:
Bug 49353: ComplexTextController not prepared to handle multiple runs
https://bugs.webkit.org/show_bug.cgi?id=49353

Attachment 73555: Proposed changes.
https://bugs.webkit.org/attachment.cgi?id=73555&action=review

------- Additional Comments from mitz at webkit.org
View in context: https://bugs.webkit.org/attachment.cgi?id=73555&action=review

Wow, the code was totally wrong. I said r+ but I think there’s room for
improvement.

> WebCore/platform/graphics/mac/ComplexTextController.h:167
> +    const int m_end;

Huh? I can’t cq+ because of this.

> WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:54
>      m_coreTextIndices = CTRunGetStringIndicesPtr(m_coreTextRun.get());
> -    if (!m_coreTextIndices) {
> +    if (!m_coreTextIndices || runRange.location) {

You can save the function call to CTRunGetStringIndicesPtr() if
runRange.location is non-zero. I’d to it like this:
m_coreTextIndices = runRange.location ? 0 :
CTRunGetStringIndicesPtr(m_coreTextRun.get())
if (!m_coreTextIndices) { …

> WebCore/platform/graphics/mac/ComplexTextControllerCoreText.cpp:58
> +	   for (unsigned i = 0; i < m_glyphCount; ++i)
> +	       m_coreTextIndicesVector[i] -= runRange.location;

This loop should arguably be inside an if (runRange.location) block.