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.
Created attachment 260779 [details] patch
Created attachment 260783 [details] patch
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?
Created attachment 260857 [details] patch
> 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.
https://trac.webkit.org/r189539