[Webkit-unassigned] [Bug 116863] Share FontGlyphs

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=116863


Andreas Kling <akling at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #203185|review?                     |review+
               Flag|                            |




--- Comment #22 from Andreas Kling <akling at apple.com>  2013-05-29 07:13:53 PST ---
(From update of attachment 203185)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.