Bug 224277

Created attachment 425379 [details] Patch

Created attachment 425383 [details] Patch

Created attachment 425384 [details] Patch

Created attachment 425385 [details] Patch

Created attachment 425389 [details] Patch

Comment on attachment 425389 [details] Patch Needs a slight refactor, I've done some bad things with references here...

Created attachment 425418 [details] Patch

Should fix those crashes before we review.

Created attachment 425485 [details] Patch

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

Created attachment 425498 [details] Patch

Created attachment 425499 [details] Patch

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

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

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

Created attachment 425611 [details] Patch

Created attachment 425613 [details] Patch

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?

Created attachment 425722 [details] Patch

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

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

<rdar://problem/76530313>