Bug 224178

The bulk of this patch I think will be providing an alternative to CachedFont/CachedResourceLoader that can work with Worker and ThreadableLoader. It might be nice to abstract CachedFont and provide interfaces on ScriptExecutionLoader to minimise changes and multiple code-paths in the CSS* classes, so that we have something along the lines of: > CSSFontFaceSrcValue: > fontRequest = ScriptExecutionContext.requestFont(...); > > CSSFontSelector: > ScriptExecutionContext.beginLoadingFontSoon(fontRequest); > > WorkerGlobalScope: > ::beginLoadingFontSoon(fontRequest) { /* TBD; Use threadable loader to start fetching font */ } > > Document: > :: beginLoadingFontSoon(fontRequest) { > auto& font = downcast<CachedFont>(fontRequest); > // Code that is currently in CSSFontSelector to deal with CachedResourceLoader > } > > :: fontLoadingTimerFired { /* Moved from CSSFontSelector */ } The FontRequest object would need a fontLoaded client interface to replace CachedFontClient and equivalents for errorOccurred, ensureCustomFontData and createFont and I think that would be about it... There's currently some complication with font loading that is mainly to do with page-load optimisation that doesn't really apply in the Worker case, so it's nice that this abstraction would move that to be contained in Document and not CSSFontSelector. I think this abstraction could be a separate patch from the Worker implementation of it, so that's how I'll start.

<rdar://problem/76529024>

I was hoping that bug 224277, bug 224426 and some loading code were the main blockers for this, but of course it turns out to be a bit less simple than that... I'm currently working through what else needs to happen to enable FontFace in Workers, there may be more bugs before the final "loader code + enable" patch.

Not as bad as I thought actually, hopefully have a patch for this tomorrow - some weird behaviour with an isLoading check in CSSFontFaceSource that seems reversed at a glance but breaks if changed (and breaks for Workers if not changed)...

Ok, I have this all working at this point, the issue was some assumed behaviour that was resolved by just adding a FontLoadRequest::isPending implementation. The last issue is that there's still some access of FontCache::singleton in the graphics code and it looks like we'll need to plumb through a FontCache reference parameter for some functions. Otherwise looking pretty good!

Created attachment 426202 [details] Patch

Created attachment 426204 [details] Patch

Created attachment 426206 [details] Patch

Damn, I resolved Font using FontCache::singleton incorrectly by having it keep a reference to a FontCache on creation, but it looks like that isn't compatible with how Mac uses fonts judging by the test results... So looks like specific functions will need changing instead. That's a shame.

Created attachment 426212 [details] Patch

Created attachment 426217 [details] Patch

Comment on attachment 426217 [details] Patch Removing review request, I'd left out FontFaceSet, which is reasonably straight-forward to fix and it's nice to know there's actually a test for this.

Created attachment 426712 [details] Patch

Created attachment 426718 [details] Patch

Comment on attachment 426718 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426718&action=review > Source/WebCore/ChangeLog:38 > + Use FontLoadRequest::isPending instead of making assumptions about > + its state on construction. Also pass the FontCache singleton where > + appropriate in font creation. Does using isPending have any observable effect? Your comment makes this change sound sensible, but it’s oblique enough that I can’t tell if you did this to solve a problem, avoid what would be a new OffscreenCanvas-only problem, or just did it to make the code more logical with no observable effect. > Source/WebCore/css/CSSFontFace.h:176 > + FontCache& fontCacheFallbackToSingleton(); I think in this function name "fall back" is the two word verb, not the single word noun, so should be spelled "FallBack". But also maybe "FallingBack". Apparently we have used this name elsewhere, same comment applies there. > Source/WebCore/css/FontFace.cpp:67 > + auto result = adoptRef(*new FontFace(*context.cssFontSelector())); What guarantees cssFontSelector is non-null? > Source/WebCore/css/FontFace.cpp:80 > + auto value = CSSPropertyParserWorkerSafe::parseFontFaceSrc(string, is<Document>(context) ? CSSParserContext(downcast<Document>(context)) : HTMLStandardMode); Can we make a helper so we don’t need to write it with ?: here? Or is this a unique situation? > Source/WebCore/platform/graphics/Font.h:97 > static Ref<Font> create(const FontPlatformData& platformData, Origin origin = Origin::Local, Interstitial interstitial = Interstitial::No, > Visibility visibility = Visibility::Visible, OrientationFallback orientationFallback = OrientationFallback::No, Optional<RenderingResourceIdentifier> identifier = WTF::nullopt) These create functions shouldn’t be inlined in the header. We should inline constructors into create functions rather than inlining create functions into call sites, and keep the create functions out of header files. > Source/WebCore/platform/graphics/Font.h:101 > + static Ref<Font> create(const FontPlatformData& platformData, Origin origin, FontCache* fontCacheForVerticalData, Interstitial interstitial = Interstitial::No, Ditto. > Source/WebCore/workers/WorkerFontLoadRequest.cpp:44 > + : m_url(url) Missing WTFMove here. > Source/WebCore/workers/WorkerFontLoadRequest.cpp:52 > + ASSERT(is<WorkerGlobalScope>(context)); > + auto& workerGlobalScope = downcast<WorkerGlobalScope>(context); Why doesn’t this take a WorkerGlobalScope& instead of a ScriptExecutionContext&? > Source/WebCore/workers/WorkerFontLoadRequest.cpp:79 > +bool WorkerFontLoadRequest::ensureCustomFontData(const AtomString&) Why does this ignore the remote URI? > Source/WebCore/workers/WorkerFontLoadRequest.cpp:89 > +#if !PLATFORM(COCOA) > + if (!m_fontCustomPlatformData && !m_errorOccurred && !m_isLoading && m_data && isWOFF(*m_data)) { > + Vector<char> convertedFont; > + if (convertWOFFToSfnt(*m_data, convertedFont)) > + m_data = SharedBuffer::create(WTFMove(convertedFont)); > + else > + m_errorOccurred = true; > + } > +#endif Can this be in shared code? How is this worker-specific? > Source/WebCore/workers/WorkerFontLoadRequest.cpp:121 > + return; I don’t think we need this return statement.. > Source/WebCore/workers/WorkerFontLoadRequest.h:29 > +#include "ResourceLoaderOptions.h" I don’t see the use of this header; maybe it’s LoadedFromOpaqueSource? > Source/WebCore/workers/WorkerFontLoadRequest.h:31 > +#include <wtf/RefPtr.h> URL.h includes RefPtr.h, so you don’t need to include this here. > Source/WebCore/workers/WorkerFontLoadRequest.h:39 > +class WorkerGlobalScope; Don’t need this; it’s not used. Although maybe it would be used if you changed the type of the argument to load as I suggest. > Source/WebCore/workers/WorkerFontLoadRequest.h:40 > +struct FontCustomPlatformData; Should leave a blank line before this. > Source/WebCore/workers/WorkerGlobalScope.cpp:528 > + return cssFontSelector()->fontFaceSet(); What guarantees cssFontSelector() is non-null? > Source/WebCore/workers/WorkerGlobalScope.h:55 > +class FontLoadRequest; This should not be needed. We are overriding a function that takes a parameter of this type, so if we were able to compile the base class declaration of the function, we should be able to compile the override without additional forward declarations. > Source/WebCore/workers/WorkerGlobalScope.h:130 > + std::unique_ptr<FontLoadRequest> fontLoadRequest(String&, bool, bool, LoadedFromOpaqueSource) final; Why does this take a String&? Even though this is just an override, I think it should mention the argument names when the type does not make clear what they are. Debatable, I suppose.

Anything not mentioned will be addressed, comments in-line. (In reply to Darin Adler from comment #15) > Comment on attachment 426718 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426718&action=review > > > Source/WebCore/ChangeLog:38 > > + Use FontLoadRequest::isPending instead of making assumptions about > > + its state on construction. Also pass the FontCache singleton where > > + appropriate in font creation. > > Does using isPending have any observable effect? Your comment makes this > change sound sensible, but it’s oblique enough that I can’t tell if you did > this to solve a problem, avoid what would be a new OffscreenCanvas-only > problem, or just did it to make the code more logical with no observable > effect. The previous code assumed that if isLoading() is false then the load request has finished, which is an odd implementation detail of how font loading currently works on a Document. With the Worker path, the font load hasn't begun at this point, so that assumption doesn't hold. Without this change, it meant that Worker-path custom fonts would never load as this code would set the the status to success and the load would then never be triggered. This change alone without the worker path has no observable effect, but I do believe this could also be a source of future confusion (at least, it took me a while to figure out what was happening here). > > Source/WebCore/css/FontFace.cpp:67 > > + auto result = adoptRef(*new FontFace(*context.cssFontSelector())); > > What guarantees cssFontSelector is non-null? cssFontSelector is guaranteed non-null for WorkerGlobalScope and Document at the moment, but this should be guarded. I've added an assert (and I've done the same for the one in WorkerGlobalScope, which I think is perhaps debatable as the implementation is in the same file, but it doesn't hurt). > > Source/WebCore/css/FontFace.cpp:80 > > + auto value = CSSPropertyParserWorkerSafe::parseFontFaceSrc(string, is<Document>(context) ? CSSParserContext(downcast<Document>(context)) : HTMLStandardMode); > > Can we make a helper so we don’t need to write it with ?: here? Or is this a > unique situation? This is a pretty unique situation. I tried a few ways of writing a helper for this, but I just ended up with more convoluted code than this. Maybe we can re-examine this down the line. > > Source/WebCore/workers/WorkerFontLoadRequest.cpp:79 > > +bool WorkerFontLoadRequest::ensureCustomFontData(const AtomString&) > > Why does this ignore the remote URI? The same thing happens in CachedFont. The URI is only used for SVG fonts (it gets used in CachedSVGFont), which aren't really possible in workers for the moment. > > Source/WebCore/workers/WorkerFontLoadRequest.cpp:89 > > +#if !PLATFORM(COCOA) > > + if (!m_fontCustomPlatformData && !m_errorOccurred && !m_isLoading && m_data && isWOFF(*m_data)) { > > + Vector<char> convertedFont; > > + if (convertWOFFToSfnt(*m_data, convertedFont)) > > + m_data = SharedBuffer::create(WTFMove(convertedFont)); > > + else > > + m_errorOccurred = true; > > + } > > +#endif > > Can this be in shared code? How is this worker-specific? This isn't worker-specific, I'll have to find a good place to put this... > > Source/WebCore/workers/WorkerFontLoadRequest.h:29 > > +#include "ResourceLoaderOptions.h" > > I don’t see the use of this header; maybe it’s LoadedFromOpaqueSource? Right. > > Source/WebCore/workers/WorkerGlobalScope.h:130 > > + std::unique_ptr<FontLoadRequest> fontLoadRequest(String&, bool, bool, LoadedFromOpaqueSource) final; > > Why does this take a String&? Even though this is just an override, I think > it should mention the argument names when the type does not make clear what > they are. Debatable, I suppose. I didn't really like this either, so I've included the arguments.

Created attachment 426796 [details] Patch

Created attachment 426797 [details] Patch

Created attachment 426799 [details] Patch

Committed r276450 (236910@main): <https://commits.webkit.org/236910@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 426799 [details].