Bug 116318 – [CSS] Minor cleanups in CSS variables handling

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Claudio Saavedra]

[CSS] Minor cleanups in CSS variables handling

[Claudio Saavedra]

Created attachment 202091 [details]
Patch

[Darin Adler]

Comment on attachment 202091 [details]
Patch

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.

[Claudio Saavedra]

(In reply to comment #2)
>     ASSERT(m_length >= length);

Isn't that implicit below?

>     ASSERT(m_length >= position + length);

[Claudio Saavedra]

Created attachment 202105 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 202105 [details]
Patch

Sorry, I should have caught this when you were landing the first patch.

[Claudio Saavedra]

Committed r150302: <http://trac.webkit.org/changeset/150302>