[webkit-reviews] review granted: [Bug 22717] Make CSS values use less memory : [Attachment 25832] share primitive value instances
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun Dec 7 15:57:20 PST 2008
Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 22717: Make CSS values use less memory
https://bugs.webkit.org/show_bug.cgi?id=22717
Attachment 25832: share primitive value instances
https://bugs.webkit.org/attachment.cgi?id=25832&action=review
------- Additional Comments from Darin Adler <darin at apple.com>
> + // 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
More information about the webkit-reviews
mailing list