[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