[webkit-reviews] review denied: [Bug 116318] [CSS] Minor cleanups in CSS variables handling : [Attachment 202091] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Claudio Saavedra
<csaavedra at igalia.com>'s request for review:
Bug 116318: [CSS] Minor cleanups in CSS variables handling
https://bugs.webkit.org/show_bug.cgi?id=116318

Attachment 202091: Patch
https://bugs.webkit.org/attachment.cgi?id=202091&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=202091&action=review


> Source/WebCore/css/CSSParser.cpp:-407
> -    ASSERT(m_length >= position + length);
> -
> -    RefPtr<StringImpl> result;
> -
> -    if (is8Bit()) {
> -	   result = StringImpl::create(characters8() + position, length);
> -    } else {
> -	   result = StringImpl::create(characters16() + position, length);
> -    }
> -
> -    return AtomicString(result);

Here’s the correct way to write this:

    ASSERT(m_length >= length);
    ASSERT(m_length >= position + length);

    if (is8Bit())
	return AtomicString(characters8() + position, length);
    return AtomicString(characters16() + position, length);

There is no reason to call StringImpl::create, and in fact that costs memory
allocation and deallocation when the string already exists in the atomic string
table.

> Source/WebCore/css/CSSParser.cpp:3313
> -    AtomicString variableName = name.substring(prefixLength, name.length() -
prefixLength);
> +    AtomicString variableName(String(name).impl(), prefixLength,
name.length() - prefixLength);

Like the old code, this does unnecessary memory allocation, so you should do
what I suggested instead.