[webkit-reviews] review granted: [Bug 215051] Unconditionally record string offsets in the fast text codepath : [Attachment 405776] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 215051: Unconditionally record string offsets in the fast text codepath
https://bugs.webkit.org/show_bug.cgi?id=215051

Attachment 405776: Patch

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




--- Comment #4 from Darin Adler <darin at apple.com> ---
Comment on attachment 405776
  --> https://bugs.webkit.org/attachment.cgi?id=405776
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.