[Webkit-unassigned] [Bug 22717] Make CSS values use less memory

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 7 15:57:20 PST 2008


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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #25832|review?                     |review+
               Flag|                            |




------- Comment #5 from darin at apple.com  2008-12-07 15:57 PDT -------
(From update of attachment 25832)
> +    // These are the empty and deleted values of the hash table.
> +    static CSSPrimitiveValue* colorTransparent = new CSSPrimitiveValue(Color::transparent);
> +    static CSSPrimitiveValue* colorWhite = new CSSPrimitiveValue(Color::white);
> +    if (rgbValue == Color::transparent)
> +        return colorTransparent;
> +    if (rgbValue == Color::white)
> +        return colorWhite;

The declarations could go inside the if statements. I think it would be
slightly more elegant.

> +    RefPtr<CSSPrimitiveValue> primitiveValue = colorValueCache->get(rgbValue);
> +    if (primitiveValue)
> +        return primitiveValue;

Saves one round of reference count churn if you return primitiveValue.release()
in the various places you're returning a RefPtr, including this one.

> +    primitiveValue = adoptRef(new CSSPrimitiveValue(rgbValue));
> +    // Just wipe out the cache and start rebuilding when it gets too big.
> +    const int maxColorCacheSize = 512;
> +    if (colorValueCache->size() > maxColorCacheSize)

This makes the name of the global a lie. You should use ">=" if you really want
it to be the max size.

> +    const int maxCachedUnitType = CSS_PX;

The maxCachedUnitType thing here is subtle. I think you need a comment saying
something about the fact that CSS_PX is the last one, and maybe even a comment
where CSS_PX is defined telling about this dependency.

> +    static RefPtr<CSSPrimitiveValue>(* integerValueCache)[maxCachedUnitType + 1] = new RefPtr<CSSPrimitiveValue>[cachedIntegerCount][maxCachedUnitType + 1];

A typedef would make this way easier to read.

> +    int intValue;
> +    if (type <= CSS_PX && value >= 0 && value < cachedIntegerCount && value == (intValue = (int)value)) {

We normally prefer C++ casts. But I'm thinking that you might want to move that
last check into a separate if statement just so you can do the assignment
without doing a cast at all.

> +    

I noticed some trailing spaces on some of the blank lines.

> -    static PassRefPtr<CSSPrimitiveValue> create(const String& value, UnitTypes type)
> -    {
> -        return adoptRef(new CSSPrimitiveValue(value, type));
> -    }

Since you left this one alone, you could leave it in the header file. I don't
have any strong feelings either way.

All my comments were quibbles, not real objections. r=me


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



More information about the webkit-unassigned mailing list