Summary: | Split mixed font GlyphPage functionality to separate class | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||
Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | mmaxfield | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
Antti Koivisto
2015-09-08 10:19:13 PDT
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. |