[Webkit-unassigned] [Bug 148965] Split mixed font GlyphPage functionality to separate class

[bugzilla-daemon at webkit.org]

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

Myles C. Maxfield <mmaxfield at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #260783|review?                     |review+
              Flags|                            |

--- Comment #3 from Myles C. Maxfield <mmaxfield at apple.com> ---
Comment on attachment 260783
  --> https://bugs.webkit.org/attachment.cgi?id=260783
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?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20150908/f7a99901/attachment.html>