Bug 215051 – Unconditionally record string offsets in the fast text codepath

• Component: New Bugs
• Status: RESOLVED FIXED

[Myles C. Maxfield]

Unconditionally record string offsets in the fast text codepath

[Myles C. Maxfield]

Created attachment 405770 [details]
Patch

[Myles C. Maxfield]

Created attachment 405773 [details]
Patch

[Myles C. Maxfield]

Created attachment 405776 [details]
Patch

[Darin Adler]

Comment on attachment 405776 [details]
Patch

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

> Source/WebCore/platform/graphics/FontCascade.cpp:1266
> -        ASSERT_WITH_SECURITY_IMPLICATION(offsetInString < textRun.length());
> +        RELEASE_ASSERT_WITH_SECURITY_IMPLICATION(offsetInString < static_cast<GlyphBufferStringOffset>(textRun.length()));

Changing this to a RELEASE_ASSERT makes it unnecessary to have it inside an if statement that checks the same thing. Just put this outside the if and don’t check this in the if.

> Source/WebCore/platform/graphics/FontCascade.cpp:1273
> +        U16_NEXT(textRun.characters16(), offsetInString, static_cast<GlyphBufferStringOffset>(textRun.length()), baseCharacter);

Can we put static_cast<GlyphBufferStringOffset>(textRun.length()) in a local variable? The long expression makes it hard to see what is going on. And we could probably convert without a cast if we used a local. And we use it twice, once here and once in the assertion.

Also, seems like this should be U16_GET, not U16_NEXT. We don’t want to bump offsetInString. And if we did, we’d have to do it in the 8-bit case above.

[Myles C. Maxfield]

Committed r265241: <https://trac.webkit.org/changeset/265241>

[Radar WebKit Bug Importer]

<rdar://problem/66507427>

[Myles C. Maxfield]

Committed r265414: <https://trac.webkit.org/changeset/265414>