Bug 49353 - ComplexTextController not prepared to handle multiple runs
Summary: ComplexTextController not prepared to handle multiple runs
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Text (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-10 16:20 PST by Ned Holbrook
Modified: 2010-11-11 08:17 PST (History)
5 users (show)

See Also:


Attachments
Proposed changes. (4.51 KB, patch)
2010-11-10 16:27 PST, Ned Holbrook
mitz: review+
Details | Formatted Diff | Diff
Changes per review. (4.43 KB, patch)
2010-11-10 21:10 PST, Ned Holbrook
mitz: review+
mitz: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ned Holbrook 2010-11-10 16:20:11 PST
If, for whatever reason, CoreText were to return multiple runs for a given text run, ComplexTextController would return incorrect results. Currently ComplexTextRun::indexAt() always returns a run-based index, but if collectComplexTextRunsForCharactersCoreText() were to generate multiple ComplexTextRun objects they would not consistently be run-based since they are just using the string index array from each run, which would start at the location of that run in the string; the same goes for ComplexTextRun::characters() and ComplexTextRun::stringLocation().
Comment 1 Ned Holbrook 2010-11-10 16:27:07 PST
Created attachment 73555 [details]
Proposed changes.
Comment 2 mitz 2010-11-10 20:48:58 PST
Comment on attachment 73555 [details]
Proposed changes.

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.
Comment 3 Ned Holbrook 2010-11-10 21:00:28 PST
(In reply to comment #2)
> (From update of attachment 73555 [details])
> 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.

I'm happy to make the changes you suggested, give me a minute to prepare a new patch.
 
> > WebCore/platform/graphics/mac/ComplexTextController.h:167
> > +    const int m_end;
> 
> Huh? I can’t cq+ because of this.

Is it against WebKit style to make immutable member variables const? It was purely a defensive change, but I'll back it out.
Comment 4 Ned Holbrook 2010-11-10 21:10:14 PST
Created attachment 73576 [details]
Changes per review.
Comment 5 mitz 2010-11-10 21:17:41 PST
Comment on attachment 73576 [details]
Changes per review.

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

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

The WebKit style guidelines say that a multi-line block needs to be in braces. I’ll fix this when I land the patch.
Comment 6 Ned Holbrook 2010-11-10 21:19:32 PST
(In reply to comment #5)
> The WebKit style guidelines say that a multi-line block needs to be in braces. I’ll fix this when I land the patch.

That's cute: check-webkit-style didn't catch this. Thanks!
Comment 7 mitz 2010-11-10 21:21:09 PST
Fixed in <http://trac.webkit.org/projects/webkit/changeset/71795>. I forgot that I said I’d fix the style so I did that in <http://trac.webkit.org/projects/webkit/changeset/71796>.
Comment 8 WebKit Review Bot 2010-11-11 00:02:11 PST
http://trac.webkit.org/changeset/71795 might have broken SnowLeopard Intel Release (Tests)
The following tests are not passing:
fast/text/atsui-spacing-features.html
Comment 9 mitz 2010-11-11 00:15:46 PST
(In reply to comment #8)
> http://trac.webkit.org/changeset/71795 might have broken SnowLeopard Intel Release (Tests)
> The following tests are not passing:
> fast/text/atsui-spacing-features.html

Investigating.
Comment 10 mitz 2010-11-11 00:43:31 PST
I am really sorry. I now think that this patch was completely wrong and the code was completely right. There are actually existing test cases where CTLineGetGlyphRuns() returns multiple runs, and they were handled correctly. I mistakenly thought that this patch was about a situation that doesn’t and currently couldn’t occur in our tests (which is why it could be allowed without a test).

I am going to revert the change.
Comment 11 mitz 2010-11-11 00:47:25 PST
All ComplexTextRuns from a given call to collectComplexTextRunsForCharactersCoreText() should reference the substring indicated by the caller to that function (which is the basis for the CTLine created in it); the indices from all CTRuns are all relative to that same string, and don’t require any adjustment.
Comment 12 mitz 2010-11-11 00:57:02 PST
Reverted in <http://trac.webkit.org/changeset/71800>.
Comment 13 Ned Holbrook 2010-11-11 08:17:06 PST
(In reply to comment #12)
> Reverted in <http://trac.webkit.org/changeset/71800>.

Clearly I misunderstood the results of some of the hacking I was doing on the controller, and I appreciate both your attentiveness to this bug and its resolution. Thanks.