[webkit-reviews] review granted: [Bug 78604] Share font-family CSS values through CSSValuePool. : [Attachment 127123] Patch v2

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Andreas Kling <kling at webkit.org>'s
request for review:
Bug 78604: Share font-family CSS values through CSSValuePool.
https://bugs.webkit.org/show_bug.cgi?id=78604

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=127123&action=review


> Source/WebCore/css/CSSParser.cpp:4284
> +    { }

Normal style is to put these on successive lines instead of on one line.

> Source/WebCore/css/CSSParser.cpp:4313
> +    FontFamilyValueBuilder familyBuilder(list.get(), cssValuePool());
> +    bool inFamily = false;

It seems that it would be easy to leave out one call to commit in the code that
follows. I the logic could be slightly clearer if more of the work was done
through builder object and it had a bit more state.

> Source/WebCore/css/CSSValuePool.h:77
> +    typedef HashMap<String, RefPtr<FontFamilyValue> > FontFamilyValueCache;

It might be better to use AtomicString for this. We could possibly save some
allocation by going directly from a parser string or a string builder to an
existing AtomicString for an existing family name rather than making a
temporary String that often ends up being used only for map lookup.
Alternatively, we could work on enhancements to HashMap<String, xxx> that could
allow us to look up in the map without allocating a new string. My guess is
that this isn’t so performance-critical that either of those matters, but it is
something I noticed while reviewing.