[webkit-reviews] review denied: [Bug 25207] Text not visible while extenal font downloading : [Attachment 67397] Patch

[bugzilla-daemon at webkit.org]

mitz at webkit.org has denied Yuzo Fujishima <yuzo at google.com>'s request for
review:
Bug 25207: Text not visible while extenal font downloading
https://bugs.webkit.org/show_bug.cgi?id=25207

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

------- Additional Comments from mitz at webkit.org
View in context:
https://bugs.webkit.org/attachment.cgi?id=67397&action=prettypatch

> WebCore/ChangeLog:5
> +	   Fix for Bug 25207 - Text not visible while extenal font downloading
Typo: “extenal”

> WebCore/ChangeLog:29
> +	   * loader/CachedFont.cpp:
> +	   (WebCore::CachedFont::beginLoadIfNeeded):
> +	   * loader/CachedResourceLoader.cpp:
> +	   (WebCore::CachedResourceLoader::CachedResourceLoader):
> +	   (WebCore::CachedResourceLoader::startFontLoadWaitLimitTimer):
> +	   (WebCore::CachedResourceLoader::fontLoadWaitLimitTimer):
> +	   * loader/CachedResourceLoader.h:
> +	   (WebCore::CachedResourceLoader::fontLoadWaitLimitExceeded):
> +	   * platform/graphics/Font.cpp:
> +	   (WebCore::Font::drawText):
> +	   * platform/graphics/FontFallbackList.cpp:
> +	   (WebCore::FontFallbackList::fontLoadWaitLimitExceeded):
> +	   * platform/graphics/FontFallbackList.h:
> +	   * platform/graphics/FontSelector.h:
There should be function-level comments about the changes.

> WebCore/platform/graphics/FontFallbackList.cpp:32
> +#include "CachedResourceLoader.h"
This is a layering violation. Platform code should not depend on higher-level
WebCore code.

> WebCore/platform/graphics/FontFallbackList.cpp:145
> +    if (m_fontSelector)
> +	   if (CachedResourceLoader* loader =
m_fontSelector->cachedResourceLoader())
> +	       if (loader->fontLoadWaitLimitExceeded())
> +		   return true;
The first and second if statements require braces.

> WebCore/platform/graphics/FontSelector.h:34
> +class CachedResourceLoader;
Again, layering violation.

This patch doesn’t implement the behavior described in
<https://bugs.webkit.org/show_bug.cgi?id=44404#c0>, but that behavior seems
preferable, especially in case the font ultimately fails to load. Has consensus
been reached on www-style regarding this?