Bug 202794

Created attachment 380642 [details] Patch

As far as I know, none of objects in StyleResolver are thread safe. We need to very carefully separate those parts that are thread safe & make things needed for this work thread safe.

Created attachment 409552 [details] Patch

A work-in-progress that I'm submitting to see failures on EWS. This patch adds CSSParser::parseFontWorkerSafe (perhaps should be called parseFontShortHandWorkerSafe...) which does the equivalent of parseColorWorkerSafe, but for font short-hands - that is, it does the CSS parsing using consume*Raw functions so as to bypass use of the CSSValuePool (which isn't thread-safe and isn't necessary as we really just want the end result, not any of the CSS gubbins). To test this new path, this also adds a bunch of code that ultimately should be shared somehow with StyleBuilder* to apply font styling from raw font style data, as opposed to CSSValue via a StyleBuilder. It'd be good to have a static function somewhere that can do this. Not handled in this WIP - Removing the need for Document, doing anything with FontResolver, anything to do with OffscreenCanvas.

Created attachment 409553 [details] Patch

Created attachment 409557 [details] Patch

Created attachment 409567 [details] Patch

Created attachment 409882 [details] Patch

Created attachment 409889 [details] Patch

I'm hijacking this bug to track what this has turned into - further work will be needed for fonts to be fully stylable in a worker, but the first step is to make CSS font shorthands safely parsable.

Created attachment 409978 [details] Patch

Some notes about this patch; This is kind of the font equivalent of bug 204575, which made full CSS colour parsing available to OffscreenCanvas. This makes font shorthand parsing available to non-main-thread users by adding 'Raw' equivalents of the necessary consume* CSS parsing functions and removing the use of CSSValue. parseFontWorkerSafe returns a FontRaw object that contains the raw values of the font longhands. To make this still useful, it also adds Style::ResolveForFontRaw, which takes a FontRaw and other necessary values and returns a RenderStyle. This removes the use of StyleBuilder, which also dependeded on using CSSValue. I've added a new file for this, somewhat in the vein of Style::ResolveForDocument. To add test coverage and reduce the difference between HTMLCanvas and OffscreenCanvas, I've used this function in CanvasRenderingContext2D::setFont, which is also, pleasantly, a code reduction in that area. There is some duplication between ResolveForFontRaw and StyleBuilderCustom/StyleBuilderConverter - not a huge amount, but I'd gladly take advice on the best way to get rid of it, as well of course as any feedback on this entire patch and design. This is most of the work necessary for OffscreenCanvas on the main thread to provide font functions, but there are still lots of not-worker-safe/enabled bits of code left to work through for font support in a Worker.

Comment on attachment 409978 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=409978&action=review > Source/WebCore/style/StyleResolveForFontRaw.cpp:49 > +Optional<RenderStyle> resolveForFontRaw(const FontRaw& fontRaw, FontCascadeDescription&& fontDescription, Document& document) > +{ > + auto fontStyle = RenderStyle::create(); RenderStyle is not thread safe. It consists of non-atomically refcounted substructures (in DataRef<> fields). For example RenderStyle::create simply clones the static default style, sharing all substructures.

(In reply to Antti Koivisto from comment #13) > Comment on attachment 409978 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=409978&action=review > > > Source/WebCore/style/StyleResolveForFontRaw.cpp:49 > > +Optional<RenderStyle> resolveForFontRaw(const FontRaw& fontRaw, FontCascadeDescription&& fontDescription, Document& document) > > +{ > > + auto fontStyle = RenderStyle::create(); > > RenderStyle is not thread safe. It consists of non-atomically refcounted > substructures (in DataRef<> fields). For example RenderStyle::create simply > clones the static default style, sharing all substructures. ok, that's good to know - in a future bug, we would need a function to either create a worker-safe RenderStyle, or to replace RenderStyle with something that could hold the same relevant data and be worker-safe. For this patch, it's not really an issue, I don't think.

Created attachment 410511 [details] Patch

Comment on attachment 410511 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=410511&action=review So much new code here. Wisher there was a way to save a bit more and make the code a bit less repetitive. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:140 > + if (category != CalculationCategory::Percent) { > + if (!allowPercentOf || (category != CalculationCategory::PercentLength > + && category != CalculationCategory::PercentNumber)) > + return WTF::nullopt; > + } This logic is confusing enough that I think it might require a helper function. I also find the bool argument here both subtle and confusing. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:217 > + if (const CSSCalcValue* calcValue = calcParser.value()) { auto > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:423 > + if (auto result = calcParser.consumePercentRaw(true)) This mysterious "true" is something we try to avoid in WebKit code. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1970 > + String familyName = concatenateFamilyName(range); > + if (familyName.isNull()) > + return WTF::nullopt; > + return familyName; This shows that Optional<String> is a strange type. Since String already has a null value built in, it’s overkill to use Optional with it, despite the fact that it seems logical to do that. Another possibilities to change concatenateFamilyName to return Optional<String>. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:2090 > +AtomString genericFontFamilyFromValueID(CSSValueID ident) This could return const AtomString&. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:2108 > + return { }; This would return nullAtom. > Source/WebCore/style/StyleBuilderCustom.h:995 > isGenericFamily = true; The old code used to say "default break". The new code instead sets isGenericFamily to true and calls CSSPropertyParserHelpers::genericFontFamilyFromValueID. Is that going to do the right thing? > Source/WebCore/style/StyleResolveForFontRaw.cpp:199 > + return static_cast<float>(CSSPrimitiveValue::computeNonCalcLengthDouble(conversionData, length.type, length.value)); Here we are casting to a float. > Source/WebCore/style/StyleResolveForFontRaw.cpp:201 > + return (parentSize * percentage) / 100.f; Here we are writing an expression that will return a double without casting to a float. Unclear why the difference.

(In reply to Darin Adler from comment #16) > Comment on attachment 410511 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410511&action=review > > So much new code here. Wisher there was a way to save a bit more and make > the code a bit less repetitive. > > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:140 > > + if (category != CalculationCategory::Percent) { > > + if (!allowPercentOf || (category != CalculationCategory::PercentLength > > + && category != CalculationCategory::PercentNumber)) > > + return WTF::nullopt; > > + } > > This logic is confusing enough that I think it might require a helper > function. I also find the bool argument here both subtle and confusing. I agree, I didn't really like this when I wrote it but nothing immediately came to mind... I'll try again. > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:217 > > + if (const CSSCalcValue* calcValue = calcParser.value()) { > > auto Will change > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:423 > > + if (auto result = calcParser.consumePercentRaw(true)) > > This mysterious "true" is something we try to avoid in WebKit code. I'll try to find something better for this (and failing that, at least an enum or something). > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1970 > > + String familyName = concatenateFamilyName(range); > > + if (familyName.isNull()) > > + return WTF::nullopt; > > + return familyName; > > This shows that Optional<String> is a strange type. Since String already has > a null value built in, it’s overkill to use Optional with it, despite the > fact that it seems logical to do that. Another possibilities to change > concatenateFamilyName to return Optional<String>. You're right, I'll change to just using String. > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:2090 > > +AtomString genericFontFamilyFromValueID(CSSValueID ident) > > This could return const AtomString&. Thanks, well spotted. > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:2108 > > + return { }; > > This would return nullAtom. I'll change this to emptyAtom() (which I assumed { } was, obviously incorrectly) > > Source/WebCore/style/StyleBuilderCustom.h:995 > > isGenericFamily = true; > > The old code used to say "default break". The new code instead sets > isGenericFamily to true and calls > CSSPropertyParserHelpers::genericFontFamilyFromValueID. Is that going to do > the right thing? The test for family.isEmpty() should mean that the default case still does the same thing with the above change. > > Source/WebCore/style/StyleResolveForFontRaw.cpp:199 > > + return static_cast<float>(CSSPrimitiveValue::computeNonCalcLengthDouble(conversionData, length.type, length.value)); > > Here we are casting to a float. > > > Source/WebCore/style/StyleResolveForFontRaw.cpp:201 > > + return (parentSize * percentage) / 100.f; > > Here we are writing an expression that will return a double without casting > to a float. Unclear why the difference. This is just a mistake, I'll add the cast to the latter.

Something else that needs fixing up, genericFontFamilyFromValueID isn't worker-safe at all. Need to figure out what to do with that... Ideally, some kind of thread-safe never destroyed AtomString would be great...

(In reply to Chris Lord from comment #18) > Something else that needs fixing up, genericFontFamilyFromValueID isn't > worker-safe at all. Need to figure out what to do with that... Ideally, some > kind of thread-safe never destroyed AtomString would be great... Just to note, I misread and this isn't required for parsing, so can be dealt with in another bug.

Created attachment 411444 [details] Patch

(In reply to Darin Adler from comment #16) > Comment on attachment 410511 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=410511&action=review > > So much new code here. Wisher there was a way to save a bit more and make > the code a bit less repetitive. The biggest cause of repetition is having to single out calc values, but I'm not sure how best to address that without some more major refactoring... The rest of these issues I've solved in what I would consider the most obvious way, and this specifically; > > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:140 > > + if (category != CalculationCategory::Percent) { > > + if (!allowPercentOf || (category != CalculationCategory::PercentLength > > + && category != CalculationCategory::PercentNumber)) > > + return WTF::nullopt; > > + } > > This logic is confusing enough that I think it might require a helper > function. I also find the bool argument here both subtle and confusing. I removed this weird logic and bool parameter and just added consumeLengthOrPercentRaw to CSSCalcParser, which makes more sense I think.

Just to note, I plan on landing this soon - I've been manually checking OffscreenCanvas test results with main-thread font functions on a subsequent patch to make sure that behaviour doesn't deviate from canvas and from expectations. The results look good, which gives me a bit of extra confidence about landing this. Worst case scenario, we can back it out anyway.

Created attachment 414439 [details] Patch

Created attachment 414446 [details] Patch

Committed r269957: <https://trac.webkit.org/changeset/269957> All reviewed patches have been landed. Closing bug and clearing flags on attachment 414446 [details].

<rdar://problem/71539772>

Comment on attachment 414446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414446&action=review > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1957 > + if (fontStyleIsWithinRange(angleInDegrees)) This is defined statically within CSSPropertyParser.cpp so this file is not buildable outside a unified build.

Comment on attachment 414446 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=414446&action=review >> Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:1957 >> + if (fontStyleIsWithinRange(angleInDegrees)) > > This is defined statically within CSSPropertyParser.cpp so this file is not buildable outside a unified build. Are you going to fix that, should I, or do you want Chris to fix it?

(In reply to Darin Adler from comment #28) > Are you going to fix that, should I, or do you want Chris to fix it? I have a few minutes so I am working on a fix, but I might not get it done fast enough.

Fixing in bug 219222.