[webkit-reviews] review granted: [Bug 116863] Share FontGlyphs : [Attachment 203185] patch 3

[bugzilla-daemon at webkit.org]

Andreas Kling <akling at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 116863: Share FontGlyphs
https://bugs.webkit.org/show_bug.cgi?id=116863

Attachment 203185: patch 3
https://bugs.webkit.org/attachment.cgi?id=203185&action=review

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


Pretty neat.
r=me with some whining:

> Source/WebCore/ChangeLog:8
> +	   Style system generates many Font objects that are indentical or
similar enough to have identical FontGlyphs. 

Typo, identical.

> Source/WebCore/ChangeLog:10
> +	   It also improves perfomrance as the glyph cache and the width cache
hang from FontGlyphs and the hit rate of

Typo, performance.

> Source/WebCore/css/CSSFontSelector.cpp:69
> +    , m_uniqueId(++fontSelectorId)

Can't this overflow? You're not even using a 64-bit type!

> Source/WebCore/css/CSSFontSelector.cpp:631
> +	   DEFINE_STATIC_LOCAL(String, webkitPrefix, ("-webkit-"));
> +	   if (familyName.startsWith(webkitPrefix))
> +	       return true;

String::startsWith() has a nice overload for this, it's better to just check
startsWith("-webkit-")

> Source/WebCore/css/CSSFontSelector.h:54
> +    virtual ~CSSFontSelector() FINAL OVERRIDE;

... na-na nah nah naaaaaa!

Sorry to ruin the '80s song, but we should just mark the class FINAL instead of
doing it to every virtual method.

> Source/WebCore/platform/graphics/Font.cpp:169
> +    if (a.fontDescriptionCacheKey != b.fontDescriptionCacheKey)
> +	   return false;
> +    if (a.families != b.families)
> +	   return false;
> +    if (a.fontSelectorId != b.fontSelectorId || a.fontSelectorVersion !=
b.fontSelectorVersion || a.fontSelectorFlags != b.fontSelectorFlags)
> +	   return false;

I would order these comparisons by cost from lowest to highest, so we can fail
faster.

> Source/WebCore/platform/graphics/Font.cpp:202
> +    if (!key.fontDescriptionCacheKey.size || key.families.isEmpty())
> +	   return 0;

The families Vector should never be empty, so I don't think this test is
correct.

> Source/WebCore/platform/graphics/Font.h:317
> +void pruneUnreferencedFromFontGlyphsCache();

Mildly awkward name. I'd call it pruneUnreferencedEntriesFromFontGlyphsCache();
(because reasons.)

> Source/WebCore/platform/graphics/FontCache.cpp:80
> +    FontDescriptionFontDataCacheKey m_fontDescriptioKey;

Typo, m_fontDescriptioKey.

> Source/WebCore/platform/graphics/FontCache.h:67
> +    FontDescriptionFontDataCacheKey(unsigned size = 0)

This should be marked explicit.