Bug 148965 – Split mixed font GlyphPage functionality to separate class

• Status: RESOLVED
• Resolution: FIXED
• Component: Text

[Antti Koivisto]

Currently GlyphPage class is used for both immutable single font case (in Font) and for caching mixed font mappings (in FontCascadeFonts). It is cleaner to use separate classed for these cases. This will also make future improvements easier.

[Antti Koivisto]

Created attachment 260779 [details]
patch

[Antti Koivisto]

Created attachment 260783 [details]
patch

[Myles C. Maxfield]

Comment on attachment 260783 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=260783&action=review

I think this is fine.

> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:56
> +void FontCascadeFonts::GlyphPageCacheEntry::setSingleFontPage(RefPtr<GlyphPage>&& page)

Shouldn't this take a Ref?

> Source/WebCore/platform/graphics/FontCascadeFonts.h:98
> +        RefPtr<GlyphPage> m_singleFont;
> +        std::unique_ptr<MixedFontGlyphPage> m_mixedFont;

Unclear why one is a RefPtr and the other one is a unique_ptr.

> Source/WebCore/platform/graphics/GlyphPage.h:74
>      static unsigned indexForCharacter(UChar32 c) { return c % GlyphPage::size; }

Shouldn't this be private (maybe with a friend of MixedFontGlyphPage?) Seems to me that external people don't need to know this.

> Source/WebCore/platform/graphics/GlyphPage.h:83
> +        return m_glyphs[indexForCharacter(c)];

Please add ASSERT_WITH_SECURITY_IMPLICATION() in all member functions which deal with this stuff

> Source/WebCore/platform/graphics/GlyphPage.h:94
> +    void setGlyphDataForIndex(unsigned index, Glyph glyph)

Looks to me like this function is just setting the glyph, not the GlyphData. Now that you are separating mixed parts from the non-mixed parts, I think it makes sense to give them different APIs.

> Source/WebCore/platform/graphics/GlyphPage.h:121
> +class MixedFontGlyphPage {

Seems to me that this should get its own file.

> Source/WebCore/platform/graphics/GlyphPage.h:124
> +    MixedFontGlyphPage(RefPtr<GlyphPage>&& initialPage)

Judging from the function body, shouldn't this take a regular lvalue reference instead of an rvalue reference?

[Antti Koivisto]

Created attachment 260857 [details]
patch

[Antti Koivisto]

> Unclear why one is a RefPtr and the other one is a unique_ptr.

One is shared, other is not.

>> +class MixedFontGlyphPage { 
> Seems to me that this should get its own file.

I moved it to FontCascadeFonts.cpp since it is really an implementation detail of that type.

[Antti Koivisto]

https://trac.webkit.org/r189539