[Webkit-unassigned] [Bug 116318] [CSS] Minor cleanups in CSS variables handling

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116318


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #202091|review?                     |review-
               Flag|                            |




--- Comment #2 from Darin Adler <darin at apple.com>  2013-05-17 09:09:07 PST ---
(From update of attachment 202091)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.