[webkit-reviews] review denied: [Bug 97993] [Chromium] Introduce caches for HarfBuzzShaper : [Attachment 166424] Patch

[bugzilla-daemon at webkit.org]

Tony Chang <tony at chromium.org> has denied Kenichi Ishibashi
<bashi at chromium.org>'s request for review:
Bug 97993: [Chromium] Introduce caches for HarfBuzzShaper
https://bugs.webkit.org/show_bug.cgi?id=97993

Attachment 166424: Patch
https://bugs.webkit.org/attachment.cgi?id=166424&action=review

------- Additional Comments from Tony Chang <tony at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=166424&action=review


Just some style nits.  The ownership of m_glyphCache is a little confusing.  It
might be more clear if you named m_glyphCache more descriptively in
HarfBuzzFontData and HarfBuzzNGFace.  Maybe something like
m_glyphCacheForFaceCacheEntry?

> Source/WebCore/ChangeLog:10
> +	   - Implement canRenderCombiningCharacterSequence() for ports which
use
> +	   HarfBuzzShaper. This function caches the result and will improve the

> +	   performance of HarfBuzzShaper::collectHarfBuzzRuns.

Please include performance numbers in the ChangeLog.  E.g., this makes the
intl2 page cycler X% faster.

> Source/WebCore/platform/graphics/freetype/SimpleFontDataFreeType.cpp:176
> +#if USE(HARFBUZZ_NG)
> +bool SimpleFontData::canRenderCombiningCharacterSequence(const UChar*,
size_t) const

Which build uses this file?  I thought Blackberry and Chromium use the
SimpleFontDataSkia.cpp.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:47
> +struct FaceCacheEntry {
> +    unsigned m_refCount;

Can we use RefCounted rather than doing our own ref counting?  The HashMap
would have RefPtr's holding 1 ref.  You could use ref/deref directly and when
the ref becomes 1, remove it from the cache (which should cause the destructor
to run and delete m_face).

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFace.cpp:68
> +	   result.iterator->second = WTF::adoptPtr(new FaceCacheEntry);
> +	   result.iterator->second->m_refCount = 0;
> +	   result.iterator->second->m_face = createFace();

I would put this into a static create() method.

> Source/WebCore/platform/graphics/harfbuzz/ng/HarfBuzzNGFaceSkia.cpp:179
> +    HarfBuzzFontData* hbFontData = new HarfBuzzFontData;
> +    hbFontData->m_glyphCache = m_glyphCache;

I would make a constructor for HarfBuzzFontData that takes m_glyphCache as a
parameter.