Bug 116318 - [CSS] Minor cleanups in CSS variables handling
Summary: [CSS] Minor cleanups in CSS variables handling
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Claudio Saavedra
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-17 08:20 PDT by Claudio Saavedra
Modified: 2013-05-17 14:53 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.68 KB, patch)
2013-05-17 08:23 PDT, Claudio Saavedra
no flags Details | Formatted Diff | Diff
Patch (2.13 KB, patch)
2013-05-17 09:59 PDT, Claudio Saavedra
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Claudio Saavedra 2013-05-17 08:20:09 PDT
[CSS] Minor cleanups in CSS variables handling
Comment 1 Claudio Saavedra 2013-05-17 08:23:24 PDT
Created attachment 202091 [details]
Patch
Comment 2 Darin Adler 2013-05-17 09:09:07 PDT
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.
Comment 3 Claudio Saavedra 2013-05-17 09:52:23 PDT
(In reply to comment #2)
>     ASSERT(m_length >= length);

Isn't that implicit below?

>     ASSERT(m_length >= position + length);
Comment 4 Claudio Saavedra 2013-05-17 09:59:17 PDT
Created attachment 202105 [details]
Patch
Comment 5 Ryosuke Niwa 2013-05-17 12:30:25 PDT
Comment on attachment 202105 [details]
Patch

Sorry, I should have caught this when you were landing the first patch.
Comment 6 Claudio Saavedra 2013-05-17 14:53:08 PDT
Committed r150302: <http://trac.webkit.org/changeset/150302>