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

[bugzilla-daemon at webkit.org]

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

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

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=67663&action=review

I looked over it quickly and noted a minor nit, but nothing major.

I'd like to look more closely before I r+, but before I do that, I don't
understand why there isn't a test for this (r- based on no test) -- more on
this in my comments.

> WebCore/ChangeLog:27
> +	   No new tests because testing dynamic loading behavior is difficult
to

It sounds like the behavior being fixed is something very deterministic (not a
race condition, etc.).

Specifically, it occurs when the font being returned takes too long. Why can't
we make the font server in the test hang while returning a result and use that
to test it? (There are some xhr tests that do this.)

> WebCore/loader/CachedFont.cpp:196
> +    CachedResourceClientWalker w(m_clients);

Don't use abbreviations "w" for variable names.

> WebCore/loader/CachedFont.cpp:197
> +    while (CachedResourceClient *c = w.next())

And "c".

> WebCore/platform/graphics/FontFallbackList.cpp:93
> +	   if (m_fontList[i].second) {

Consider using "continue" to avoid deep nesting.