[webkit-reviews] review granted: [Bug 221064] Remove some uses of FontSelector from within CSSFontFace : [Attachment 418588] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 221064: Remove some uses of FontSelector from within CSSFontFace
https://bugs.webkit.org/show_bug.cgi?id=221064

Attachment 418588: Patch

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




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

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

> Source/WebCore/css/CSSFontFace.cpp:98
> +    , m_shouldIgnoreFontLoadCompletions(m_fontSelector &&
m_fontSelector->document() ?
m_fontSelector->document()->settings().shouldIgnoreFontLoadCompletions() :
false)

I suggest using && here more instead of using ?: with false.

I suggest using fontSelector here instead of m_fontSelector since you plan to
remove the latter.

> Source/WebCore/css/CSSFontFace.cpp:100
> +    , m_allowUserInstalledFonts(m_fontSelector && m_fontSelector->document()
&& !m_fontSelector->document()->settings().shouldAllowUserInstalledFonts() ?
AllowUserInstalledFonts::No : AllowUserInstalledFonts::Yes)

I suggest using fontSelector here instead of m_fontSelector since you plan to
remove the latter.

I think we should try to find a way to get to settings that works even when the
font selector is nullptr. I think we should also restructure things so the
Settings& is an argument to the constructor. We can have one constructor call
another, and the one that takes Settings& can be private.

> Source/WebCore/css/CSSFontFace.h:156
> +    bool shouldIgnoreFontLoadCompletions() const { return
m_shouldIgnoreFontLoadCompletions; };

Unneeded semicolon here after the closing brace.