Bug 148965

Summary: Split mixed font GlyphPage functionality to separate class
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: TextAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: mmaxfield
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
mmaxfield: review+
patch none

Description Antti Koivisto 2015-09-08 10:19:13 PDT
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.
Comment 1 Antti Koivisto 2015-09-08 11:59:28 PDT
Created attachment 260779 [details]
patch
Comment 2 Antti Koivisto 2015-09-08 12:46:36 PDT
Created attachment 260783 [details]
patch
Comment 3 Myles C. Maxfield 2015-09-08 14:47:36 PDT
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?
Comment 4 Antti Koivisto 2015-09-09 04:41:48 PDT
Created attachment 260857 [details]
patch
Comment 5 Antti Koivisto 2015-09-09 04:43:36 PDT
> 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.
Comment 6 Antti Koivisto 2015-09-09 06:01:38 PDT
https://trac.webkit.org/r189539