[webkit-reviews] review denied: [Bug 42154] Web Font is downloaded even when all the characters in the document are outside its unicode-range : [Attachment 79258] Address comment #25

[bugzilla-daemon at webkit.org]

Dave Hyatt <hyatt at apple.com> has denied Yuzo Fujishima <yuzo at google.com>'s
request for review:
Bug 42154: Web Font is downloaded even when all the characters in the document
are outside its unicode-range
https://bugs.webkit.org/show_bug.cgi?id=42154

Attachment 79258: Address comment #25
https://bugs.webkit.org/attachment.cgi?id=79258&action=review

------- Additional Comments from Dave Hyatt <hyatt at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=79258&action=review

Looks decent. You can avoid having to change SimpleFontData (and all the
platform-specific files) with my suggestions below and make the patch much
simpler/smaller.

> Source/WebCore/css/CSSFontFaceSource.cpp:181
> +	   // Store cached font and cached resource loader in SimpleFontData to
be used for on-demand web font loading.
> +	   fontData.set(new SimpleFontData(tempData->platformData(), true,
true, m_font.get(), fontSelector->cachedResourceLoader()));

You don't need to cache the resource loader in the FontData.

> Source/WebCore/platform/graphics/FontFastPath.cpp:79
> +		   GlyphData data = page->glyphDataForCharacter(c,
CharacterFromDocument);

Make a static helper function in FontFastPath.cpp that takes the Font and the
Page and the character and hands you back the GlyphData. The Font is able to
get to the cached resource loader, since the Font can get to the FontSelector.
This way you don't have to cache the resource loader ever, and you also don't
need to patch glyphDataForCharacter.

> Source/WebCore/platform/graphics/GlyphPageTreeNode.cpp:57
> +GlyphData GlyphPage::glyphDataForCharacter(UChar32 c, CharacterSource
characterSource) const
> +{
> +    unsigned index = indexForCharacter(c);
> +
> +    if (characterSource == CharacterFromDocument)
> +	   if (const SimpleFontData* fontData = m_glyphFontData[index])
> +	       fontData->beginLoadIfNeeded();
> +
> +    return GlyphData(m_glyphs[index], m_glyphFontData[index]);
> +}

This can just be a static function in FontFastPath.cpp, and you don't need this
CharacterSource stuff.

> Source/WebCore/platform/graphics/SimpleFontData.cpp:63
> +    , m_isUsed(false)
> +    , m_font(font)
> +    , m_cachedResourceLoader(cachedResourceLoader)

You shouldn't need any of this.

> Source/WebCore/platform/graphics/SimpleFontData.cpp:219
> +void SimpleFontData::beginLoadIfNeeded() const
> +{
> +    if (!m_isUsed && isCustomFont() && isLoading() && m_font &&
m_cachedResourceLoader) {
> +	   m_isUsed = true;
> +	   m_font->beginLoadIfNeeded(m_cachedResourceLoader);
> +    }
> +}

Can just be in FontFastPath instead in the helper function.

> Source/WebCore/platform/graphics/SimpleFontData.h:75
> +    SimpleFontData(const FontPlatformData&, bool isCustomFont = false, bool
isLoading = false, CachedFont* = 0, CachedResourceLoader* = 0);

See previous comments, but you shouldn't have to change SimpleFontData at all.