Bug 224277 – Refactor font loading to make it possible for Worker to implement it

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Chris Lord]

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.

[Chris Lord]

Created attachment 425379 [details]
Patch

[Chris Lord]

Created attachment 425383 [details]
Patch

[Chris Lord]

Created attachment 425384 [details]
Patch

[Chris Lord]

Created attachment 425385 [details]
Patch

[Chris Lord]

Created attachment 425389 [details]
Patch

[Chris Lord]

Comment on attachment 425389 [details]
Patch

Needs a slight refactor, I've done some bad things with references here...

[Chris Lord]

Created attachment 425418 [details]
Patch

[Darin Adler]

Should fix those crashes before we review.

[Chris Lord]

Created attachment 425485 [details]
Patch

[Chris Lord]

Fixed. I'd missed checking a result - took the opportunity to remove some unnecessary pointer use in the API.

[Chris Lord]

Created attachment 425498 [details]
Patch

[Chris Lord]

Created attachment 425499 [details]
Patch

[Darin Adler]

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++.

[Chris Lord]

(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.

[Chris Lord]

(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.

[Chris Lord]

Created attachment 425611 [details]
Patch

[Chris Lord]

Created attachment 425613 [details]
Patch

[Darin Adler]

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?

[Chris Lord]

Created attachment 425722 [details]
Patch

[Chris Lord]

(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.

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/76530313>