<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mmaxfield@apple.com" title="Myles C. Maxfield <mmaxfield@apple.com>"> <span class="fn">Myles C. Maxfield</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - Split mixed font GlyphPage functionality to separate class"
href="https://bugs.webkit.org/show_bug.cgi?id=148965">bug 148965</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #260783 Flags</td>
<td>review?
</td>
<td>review+
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - Split mixed font GlyphPage functionality to separate class"
href="https://bugs.webkit.org/show_bug.cgi?id=148965#c3">Comment # 3</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - Split mixed font GlyphPage functionality to separate class"
href="https://bugs.webkit.org/show_bug.cgi?id=148965">bug 148965</a>
from <span class="vcard"><a class="email" href="mailto:mmaxfield@apple.com" title="Myles C. Maxfield <mmaxfield@apple.com>"> <span class="fn">Myles C. Maxfield</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=260783&action=diff" name="attach_260783" title="patch">attachment 260783</a> <a href="attachment.cgi?id=260783&action=edit" title="patch">[details]</a></span>
patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=260783&action=review">https://bugs.webkit.org/attachment.cgi?id=260783&action=review</a>
I think this is fine.
<span class="quote">> Source/WebCore/platform/graphics/FontCascadeFonts.cpp:56
> +void FontCascadeFonts::GlyphPageCacheEntry::setSingleFontPage(RefPtr<GlyphPage>&& page)</span >
Shouldn't this take a Ref?
<span class="quote">> Source/WebCore/platform/graphics/FontCascadeFonts.h:98
> + RefPtr<GlyphPage> m_singleFont;
> + std::unique_ptr<MixedFontGlyphPage> m_mixedFont;</span >
Unclear why one is a RefPtr and the other one is a unique_ptr.
<span class="quote">> Source/WebCore/platform/graphics/GlyphPage.h:74
> static unsigned indexForCharacter(UChar32 c) { return c % GlyphPage::size; }</span >
Shouldn't this be private (maybe with a friend of MixedFontGlyphPage?) Seems to me that external people don't need to know this.
<span class="quote">> Source/WebCore/platform/graphics/GlyphPage.h:83
> + return m_glyphs[indexForCharacter(c)];</span >
Please add ASSERT_WITH_SECURITY_IMPLICATION() in all member functions which deal with this stuff
<span class="quote">> Source/WebCore/platform/graphics/GlyphPage.h:94
> + void setGlyphDataForIndex(unsigned index, Glyph glyph)</span >
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.
<span class="quote">> Source/WebCore/platform/graphics/GlyphPage.h:121
> +class MixedFontGlyphPage {</span >
Seems to me that this should get its own file.
<span class="quote">> Source/WebCore/platform/graphics/GlyphPage.h:124
> + MixedFontGlyphPage(RefPtr<GlyphPage>&& initialPage)</span >
Judging from the function body, shouldn't this take a regular lvalue reference instead of an rvalue reference?</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>