[webkit-reviews] review granted: [Bug 65123] Font loading during layout can cause layout code to be re-entered via resource load delegate : [Attachment 101952] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted mitz at webkit.org's request for review:
Bug 65123: Font loading during layout can cause layout code to be re-entered
via resource load delegate
https://bugs.webkit.org/show_bug.cgi?id=65123

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=101952&action=review


> Source/WebCore/css/CSSFontFaceSource.cpp:178
> +	   // the middle of layout, and the loader may invoke aribtrary
delegate or event handler code.

typo: aribtrary

> Source/WebCore/css/CSSFontFaceSource.cpp:181
> +	   if (!m_startLoadingTimer.isActive())
> +	       m_startLoadingTimer.startOneShot(0);

Sometimes it seems to me that these zero-length timers aren’t really timers.

> Source/WebCore/css/CSSFontFaceSource.cpp:203
> +    if (CachedResourceLoader* cachedResourceLoader =
m_fontSelector->cachedResourceLoader())
> +	   m_font->beginLoadIfNeeded(cachedResourceLoader);

Could just call it loader.

> Source/WebCore/css/CSSFontFaceSource.h:81
> +    Timer<CSSFontFaceSource> m_startLoadingTimer;

That’s a verb phrase, not a noun phrase. Maybe m_loadStartTimer?

> LayoutTests/ChangeLog:10
> +	   Unfortunately, font loading does not fire any DOM events, so in most
cases
> +	   a constant timeout had to be introduced.

Wah! Well at least the timeout is only for test failures.