[webkit-reviews] review granted: [Bug 117715] Reduce CSSProperty's StylePropertyMetadata memory footprint by half when used inside a ImmutableStylePropertySet. : [Attachment 205220] Patch

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Alexis Menard (darktears)
<alexis at webkit.org>'s request for review:
Bug 117715: Reduce CSSProperty's StylePropertyMetadata memory footprint by half
when used inside a ImmutableStylePropertySet.
https://bugs.webkit.org/show_bug.cgi?id=117715

Attachment 205220: Patch
https://bugs.webkit.org/attachment.cgi?id=205220&action=review

------- Additional Comments from Andreas Kling <akling at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=205220&action=review


r=me.

I think this patch is an interesting step in the right direction. I'm sure we
can refine it more over time.

> Source/WebCore/ChangeLog:6
> +	   Today CSSProperty holds its metadata in the following way :

I feel like this ChangeLog would be a lot more digestible if you simply made an
"ASCII art" diagram of the new memory layout.

> Source/WebCore/css/StylePropertyShorthand.cpp:627
> +const Vector<const StylePropertyShorthand*>
matchingShorthandsForLonghand(CSSPropertyID propertyID)

Can we make this return a reference instead of a Vector by value?

> Source/WebCore/css/StylePropertyShorthand.h:60
> +    CSSPropertyID id() const { return m_shorthandID; }

I'd call this shorthandID() rather than id().

> Source/WebCore/css/makeprop.pl:35
> +die "We've reached more than 1024 CSS properties, please make sure to update
CSSProperty/StylePropertyMetadata accordingly" if (scalar(@NAMES) > 1024);

Great idea putting this here.