[webkit-reviews] review granted: [Bug 224277] Refactor font loading to make it possible for Worker to implement it : [Attachment 425613] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 224277: Refactor font loading to make it possible for Worker to implement
it
https://bugs.webkit.org/show_bug.cgi?id=224277

Attachment 425613: Patch

https://bugs.webkit.org/attachment.cgi?id=425613&action=review




--- Comment #18 from Darin Adler <darin at apple.com> ---
Comment on attachment 425613
  --> https://bugs.webkit.org/attachment.cgi?id=425613
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=425613&action=review

> Source/WebCore/css/CSSFontFaceSource.cpp:240
> -    return m_hasSVGFontFaceElement || is<CachedSVGFont>(m_font.get());
> +    if (!m_hasSVGFontFaceElement ||
!is<CachedFontLoadRequest>(m_fontRequest.get()))
> +	   return false;
> +
> +    auto* cachedFontRequest =
downcast<CachedFontLoadRequest>(m_fontRequest.get());
> +    return is<CachedSVGFont>(cachedFontRequest->cachedFont());

I’d write it like this:

    return m_hasSVGFontFaceElement
	&& is<CachedFontLoadRequest>(m_fontRequest.get())
	&&
is<CachedSVGFont>(downcast<CachedFontLoadRequest>(*m_fontRequest).cachedFont())
;

For me this seems easier to follow than the if statement and the local
variable, and also it’s slightly nicer to use reference instead of pointers
once we have null-checked something.

But also, when writing it like this, it becomes clear that the old logic used
|| and this new logic uses &&. Is this correct?