Bug 224277 - Refactor font loading to make it possible for Worker to implement it
Summary: Refactor font loading to make it possible for Worker to implement it
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Lord
URL:
Keywords: InRadar
Depends on:
Blocks: 224178
  Show dependency treegraph
 
Reported: 2021-04-07 03:50 PDT by Chris Lord
Modified: 2021-04-12 02:27 PDT (History)
19 users (show)

See Also:


Attachments
Patch (44.26 KB, patch)
2021-04-07 04:31 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (44.44 KB, patch)
2021-04-07 04:49 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (47.78 KB, patch)
2021-04-07 04:57 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (48.65 KB, patch)
2021-04-07 05:06 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (49.81 KB, patch)
2021-04-07 06:09 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (50.45 KB, patch)
2021-04-07 10:40 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (50.46 KB, patch)
2021-04-08 02:21 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (50.67 KB, patch)
2021-04-08 04:50 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (50.77 KB, patch)
2021-04-08 04:52 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (61.83 KB, patch)
2021-04-09 05:00 PDT, Chris Lord
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (61.83 KB, patch)
2021-04-09 05:10 PDT, Chris Lord
no flags Details | Formatted Diff | Diff
Patch (61.75 KB, patch)
2021-04-12 01:06 PDT, Chris Lord
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Lord 2021-04-07 03:50:38 PDT
To support FontFace in OffscreenCanvas correctly, custom font loading needs to be possible on a Worker. Currently, custom font loading is tied to CachedFont, CachedResourceLoader and Document. It would be good to abstract this somewhat to make it easier to provide an alternative implementation of the loading portion that can be used by a Worker. I detailed a plan for this in bug 224178 and have a patch incoming so we can begin testing/discussion/review.
Comment 1 Chris Lord 2021-04-07 04:31:34 PDT
Created attachment 425379 [details]
Patch
Comment 2 Chris Lord 2021-04-07 04:49:46 PDT
Created attachment 425383 [details]
Patch
Comment 3 Chris Lord 2021-04-07 04:57:58 PDT
Created attachment 425384 [details]
Patch
Comment 4 Chris Lord 2021-04-07 05:06:22 PDT
Created attachment 425385 [details]
Patch
Comment 5 Chris Lord 2021-04-07 06:09:28 PDT
Created attachment 425389 [details]
Patch
Comment 6 Chris Lord 2021-04-07 09:18:05 PDT
Comment on attachment 425389 [details]
Patch

Needs a slight refactor, I've done some bad things with references here...
Comment 7 Chris Lord 2021-04-07 10:40:59 PDT
Created attachment 425418 [details]
Patch
Comment 8 Darin Adler 2021-04-07 13:15:11 PDT
Should fix those crashes before we review.
Comment 9 Chris Lord 2021-04-08 02:21:43 PDT
Created attachment 425485 [details]
Patch
Comment 10 Chris Lord 2021-04-08 04:19:44 PDT
Fixed. I'd missed checking a result - took the opportunity to remove some unnecessary pointer use in the API.
Comment 11 Chris Lord 2021-04-08 04:50:03 PDT
Created attachment 425498 [details]
Patch
Comment 12 Chris Lord 2021-04-08 04:52:33 PDT
Created attachment 425499 [details]
Patch
Comment 13 Darin Adler 2021-04-08 08:49:01 PDT
Comment on attachment 425499 [details]
Patch

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

> Source/WebCore/css/CSSFontFaceSource.h:53
> +    CSSFontFaceSource(CSSFontFace& owner, const String& familyNameOrURI, CSSFontSelector&, std::unique_ptr<FontLoadRequest>&&);

Since the argument can’t be null, consider using UniqueRef&& instead of unique_ptr&&.

> Source/WebCore/css/CSSFontFaceSrcValue.h:70
> +    std::unique_ptr<FontLoadRequest> fontLoadRequest(ScriptExecutionContext*, bool isSVG, bool isInitiatingElementInUserAgentShadowTree);

Since this never returns nullptr, consider returning UniqueRef instead of unique_ptr.

> Source/WebCore/dom/Document.cpp:646
> +    , m_fontLoadingTimer(*this, &Document::fontLoadingTimerFired)

This is unfortunate, bloating the already huge Document class even more by adding back font function and data members  that were factored out into a separate object. Can we keep this code from CSSFontSelector in a separate object even if Document is the only client?

> Source/WebCore/dom/ScriptExecutionContext.h:170
> +    virtual std::unique_ptr<FontLoadRequest> fontLoadRequest(String& url, bool isSVG, bool isInitiatingElementInUserAgentShadowTree, LoadedFromOpaqueSource);

A function like this that returns a heap alloca\ted object would normally be named createXXX.

> Source/WebCore/loader/FontLoadRequest.h:41
> +class FontLoadRequestClient {

Can we use a function for the font loaded call\back instead of a client class? Use WTF::CompletionHandler perhaps? We used to use classes for this but we have found we don’t need that in modern C++.
Comment 14 Chris Lord 2021-04-09 02:33:56 PDT
(In reply to Darin Adler from comment #13)
> Comment on attachment 425499 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=425499&action=review
> 
> > Source/WebCore/css/CSSFontFaceSource.h:53
> > +    CSSFontFaceSource(CSSFontFace& owner, const String& familyNameOrURI, CSSFontSelector&, std::unique_ptr<FontLoadRequest>&&);
> 
> Since the argument can’t be null, consider using UniqueRef&& instead of
> unique_ptr&&.

Sounds good.

> > Source/WebCore/css/CSSFontFaceSrcValue.h:70
> > +    std::unique_ptr<FontLoadRequest> fontLoadRequest(ScriptExecutionContext*, bool isSVG, bool isInitiatingElementInUserAgentShadowTree);
> 
> Since this never returns nullptr, consider returning UniqueRef instead of
> unique_ptr.

This can be null as Document::fontLoadRequest can return nullptr.

> > Source/WebCore/dom/Document.cpp:646
> > +    , m_fontLoadingTimer(*this, &Document::fontLoadingTimerFired)
> 
> This is unfortunate, bloating the already huge Document class even more by
> adding back font function and data members  that were factored out into a
> separate object. Can we keep this code from CSSFontSelector in a separate
> object even if Document is the only client?

I'll try factoring this out into DocumentFontLoader or something like that.

> > Source/WebCore/dom/ScriptExecutionContext.h:170
> > +    virtual std::unique_ptr<FontLoadRequest> fontLoadRequest(String& url, bool isSVG, bool isInitiatingElementInUserAgentShadowTree, LoadedFromOpaqueSource);
> 
> A function like this that returns a heap alloca\ted object would normally be
> named createXXX.
> 
> > Source/WebCore/loader/FontLoadRequest.h:41
> > +class FontLoadRequestClient {
> 
> Can we use a function for the font loaded call\back instead of a client
> class? Use WTF::CompletionHandler perhaps? We used to use classes for this
> but we have found we don’t need that in modern C++.

Sure.
Comment 15 Chris Lord 2021-04-09 02:59:53 PDT
(In reply to Chris Lord from comment #14)
> (In reply to Darin Adler from comment #13)
> > Comment on attachment 425499 [details]
> > > Source/WebCore/loader/FontLoadRequest.h:41
> > > +class FontLoadRequestClient {
> > 
> > Can we use a function for the font loaded call\back instead of a client
> > class? Use WTF::CompletionHandler perhaps? We used to use classes for this
> > but we have found we don’t need that in modern C++.
> 
> Sure.

So looking at implementing this, it's tricky without just having another setCompletionHandler function, which seems against how CompletionHandler is generally used elsewhere. The issue is that FontLoadRequest isn't expected to start loading on creation and the actual begin-loading trigger is handled externally to the request, so there's no call for which you could add a CompletionHandler argument.

Ideally, ScriptExecutionContext::beginLoadingFontSoon would have this argument, but this would then mean that Document (or DocumentFontLoader) would have to save a map of FontLoadRequest->CompletionHandler, which seems overkill compared to the current Client setup. I'm open to suggestions, but it seems to me the client setup is actually quite convenient in this particular case.
Comment 16 Chris Lord 2021-04-09 05:00:13 PDT
Created attachment 425611 [details]
Patch
Comment 17 Chris Lord 2021-04-09 05:10:14 PDT
Created attachment 425613 [details]
Patch
Comment 18 Darin Adler 2021-04-10 12:11:14 PDT
Comment on attachment 425613 [details]
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?
Comment 19 Chris Lord 2021-04-12 01:06:56 PDT
Created attachment 425722 [details]
Patch
Comment 20 Chris Lord 2021-04-12 01:08:20 PDT
(In reply to Darin Adler from comment #18)
> Comment on attachment 425613 [details]
> 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?

Well spotted, it isn't correct - fixed.
Comment 21 EWS 2021-04-12 02:26:49 PDT
Committed r275817 (236387@main): <https://commits.webkit.org/236387@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425722 [details].
Comment 22 Radar WebKit Bug Importer 2021-04-12 02:27:20 PDT
<rdar://problem/76530313>