Bug 221064 - Remove some uses of FontSelector from within CSSFontFace
Summary: Remove some uses of FontSelector from within CSSFontFace
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 208351
  Show dependency treegraph
 
Reported: 2021-01-27 15:17 PST by Myles C. Maxfield
Modified: 2021-01-28 19:50 PST (History)
8 users (show)

See Also:


Attachments
Patch (5.17 KB, patch)
2021-01-27 15:19 PST, Myles C. Maxfield
darin: review+
Details | Formatted Diff | Diff
Patch for committing (5.95 KB, patch)
2021-01-27 16:01 PST, Myles C. Maxfield
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-01-27 15:17:55 PST
Remove some uses of FontSelector from within CSSFontFace
Comment 1 Myles C. Maxfield 2021-01-27 15:19:09 PST
Created attachment 418588 [details]
Patch
Comment 2 Darin Adler 2021-01-27 15:40:24 PST
Comment on attachment 418588 [details]
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.
Comment 3 Myles C. Maxfield 2021-01-27 16:01:21 PST
Created attachment 418595 [details]
Patch for committing
Comment 4 EWS 2021-01-27 18:26:23 PST
ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.
Comment 5 Myles C. Maxfield 2021-01-28 19:49:11 PST
Committed r272045: <https://trac.webkit.org/changeset/272045>
Comment 6 Radar WebKit Bug Importer 2021-01-28 19:50:15 PST
<rdar://problem/73736861>