Bug 224426

Created attachment 426004 [details] Patch

Created attachment 426007 [details] Patch

Created attachment 426012 [details] Patch

Comment on attachment 426012 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426012&action=review I think just "pool" or "valuePool" is a fine name for arguments or variables of type CSSValuePool. > Source/WebCore/ChangeLog:11 > + new CSSPropertyParserWorkerSafe and make sure they're safe to use in a Not 100% thrilled with the name "WorkerSafe". I feel about as good about it as the term "ThreadSafe"; both seem insufficiently precise concepts to me. To make this all win/win, and not *just* about fonts in onscreen canvas, it would be great to make sure these separate property parsers are easy to use in other cases too. Firing up the full CSS parser to parse once specific property might be an unwanted pattern elsewhere too, unnecessarily inefficient for some uses. > Source/WebCore/css/FontFace.cpp:84 > - auto value = FontFace::parseString(string, CSSPropertySrc); > - if (!is<CSSValueList>(value)) > - return Exception { SyntaxError }; > - CSSFontFace::appendSources(result->backing(), downcast<CSSValueList>(*value), &document, false); > - return { }; > + if (auto value = CSSPropertyParserWorkerSafe::parseFontFaceSrc(string, CSSParserContext(document))) { > + CSSFontFace::appendSources(result->backing(), *value, &document, false); > + return { }; > + } > + return Exception { SyntaxError }; Better: Guarding a local variable with a if. Worse: Making the failure case the "main code" and the success case the guarded block. There’s a clarity reason we prefer early return; too bad it’s incompatible with guarding locals with if. The call to CSSPropertyParserWorkerSafe::parseFontFaceSrc is so much more verbose than the FontFace::parseString call with a really long class name. And also, the need to explicitly write out CSSParserContext. Is there some way we can keep this idiom a little simpler? > Source/WebCore/css/FontFace.cpp:178 > + if (familyNameToUse.contains('\'') && is<Document>(context) && downcast<Document>(context).quirks().shouldStripQuotationMarkInFontFaceSetFamily()) Seems like code running in workers will need quirks too, eventually, for the same reason we need them for documents. Probably should inherit them from the document it’s created from. This code change here is a stopgap measure. > Source/WebCore/css/FontFace.cpp:196 > - if (auto value = parseString(style, CSSPropertyFontStyle)) { > + if (auto value = CSSPropertyParserWorkerSafe::parseFontFaceStyle(style, HTMLStandardMode, context.cssValuePool())) { Frustrating that this code gets so verbose. Any way to cut down on the arguments? > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:100 > + explicit CalcParser(CSSParserTokenRange& range, CalculationCategory destinationCategory, ValueRange valueRange = ValueRangeAll, CSSValuePool& cssValuePool = CSSValuePool::singleton()) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:203 > + CSSValuePool& m_cssValuePool; This member name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:431 > +RefPtr<CSSPrimitiveValue> consumePercentWorkerSafe(CSSParserTokenRange& range, ValueRange valueRange, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:536 > RefPtr<CSSPrimitiveValue> consumeAngle(CSSParserTokenRange& range, CSSParserMode cssParserMode, UnitlessQuirk unitless, UnitlessZeroQuirk unitlessZero) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:541 > +RefPtr<CSSPrimitiveValue> consumeAngleWorkerSafe(CSSParserTokenRange& range, CSSParserMode cssParserMode, CSSValuePool& cssValuePool, UnitlessQuirk unitless, UnitlessZeroQuirk unitlessZero) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:649 > +RefPtr<CSSPrimitiveValue> consumeIdentWorkerSafe(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserHelpers.cpp:3023 > +Optional<FontRaw> consumeFontRaw(CSSParserTokenRange& range, CSSParserMode cssParserMode) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:50 > +using namespace CSSPropertyParserHelpers; > +using namespace CSSPropertyParserHelpersWorkerSafe; We should typically strive to avoid these kinds of "using namespace". I’m not sure what the namespaces achieve exactly, so I’m not sure what to recommend. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:52 > +Optional<FontRaw> CSSPropertyParserWorkerSafe::parseFont(const String& string, CSSParserMode cssParserMode) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:87 > +RefPtr<CSSValue> CSSPropertyParserWorkerSafe::parseFontFaceStyle(const String& string, const CSSParserContext& context, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:106 > +RefPtr<CSSValue> CSSPropertyParserWorkerSafe::parseFontFaceWeight(const String& string, const CSSParserContext& context, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:125 > +RefPtr<CSSValue> CSSPropertyParserWorkerSafe::parseFontFaceStretch(const String& string, const CSSParserContext& context, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:144 > +RefPtr<CSSValue> CSSPropertyParserWorkerSafe::parseFontFaceUnicodeRange(const String& string, const CSSParserContext& context) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:158 > +RefPtr<CSSValue> CSSPropertyParserWorkerSafe::parseFontFaceFeatureSettings(const String& string, const CSSParserContext& context, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:172 > +RefPtr<CSSValue> CSSPropertyParserWorkerSafe::parseFontFaceDisplay(const String& string, const CSSParserContext& context, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:186 > +namespace CSSPropertyParserHelpersWorkerSafe { Not sure why we need a namespace here. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:248 > +RefPtr<CSSFontStyleValue> consumeFontStyle(CSSParserTokenRange& range, CSSParserMode cssParserMode, CSSValuePool& cssValuePool) These argument names do not need "css" in them. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:263 > +static RefPtr<CSSPrimitiveValue> consumeFontStyleKeywordValue(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:268 > +RefPtr<CSSFontStyleRangeValue> consumeFontStyleRange(CSSParserTokenRange& range, CSSParserMode cssParserMode, CSSValuePool& cssValuePool) These argument names do not need "css" in them. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:305 > +static RefPtr<CSSPrimitiveValue> consumeFontWeightAbsoluteKeywordValue(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:311 > +RefPtr<CSSValue> consumeFontWeightAbsoluteRange(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:329 > +RefPtr<CSSPrimitiveValue> consumeFontWeightAbsolute(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:337 > +RefPtr<CSSPrimitiveValue> consumeFontStretchKeywordValue(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:351 > +RefPtr<CSSPrimitiveValue> consumeFontStretch(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:363 > +RefPtr<CSSValue> consumeFontStretchRange(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:435 > +RefPtr<CSSValue> consumeFontFeatureSettings(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:449 > +RefPtr<CSSPrimitiveValue> consumeFontFaceFontDisplay(CSSParserTokenRange& range, CSSValuePool& cssValuePool) This argument name does not need "css" in it. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.h:42 > +namespace CSSPropertyParserHelpers { Not sure why this needs to go in a namespace. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.h:60 > +namespace CSSPropertyParserHelpersWorkerSafe { Not sure why these need to go in a namespace. > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.h:76 > +#if ENABLE(VARIATION_FONTS) > +RefPtr<CSSFontStyleRangeValue> consumeFontStyleRange(CSSParserTokenRange&, CSSParserMode, CSSValuePool&); > +RefPtr<CSSValue> consumeFontWeightAbsoluteRange(CSSParserTokenRange&, CSSValuePool&); > +RefPtr<CSSValue> consumeFontStretchRange(CSSParserTokenRange&, CSSValuePool&); > +#endif > +#if !ENABLE(VARIATION_FONTS) > +RefPtr<CSSPrimitiveValue> consumeFontWeightAbsolute(CSSParserTokenRange&, CSSValuePool&); > +#endif Some blank lines would make the paragraphing easier to read I think. > Source/WebCore/dom/ScriptExecutionContext.cpp:240 > +CSSValuePool& ScriptExecutionContext::cssValuePool() > +{ > + return CSSValuePool::singleton(); > +} Maybe this should go in Document and it should be pure virtual in ScriptExecutionContext? > Source/WebCore/dom/ScriptExecutionContext.h:171 > + virtual CSSValuePool& cssValuePool(); Not sure I fully understand why each script execution context has its own CSS value pool.

(In reply to Darin Adler from comment #4) > Comment on attachment 426012 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426012&action=review > > I think just "pool" or "valuePool" is a fine name for arguments or variables > of type CSSValuePool. > > > Source/WebCore/ChangeLog:11 > > + new CSSPropertyParserWorkerSafe and make sure they're safe to use in a > > Not 100% thrilled with the name "WorkerSafe". I feel about as good about it > as the term "ThreadSafe"; both seem insufficiently precise concepts to me. > > To make this all win/win, and not *just* about fonts in onscreen canvas, it > would be great to make sure these separate property parsers are easy to use > in other cases too. Firing up the full CSS parser to parse once specific > property might be an unwanted pattern elsewhere too, unnecessarily > inefficient for some uses. I'm fully agreed here, I'm also not really a fan of the term "WorkerSafe"... I suppose what this is really trying to say is that this code will not use data outside of the scope of the passed in variables, but I can't immediately think of a better/shorter phrase to encapsulate that. This doesn't really have anything to do with Workers specifically, and I don't like that the term "WorkerSafe" sort of implies that it does (which makes non-WorkerSafe variants that chain these functions feel a little odd, perhaps). But I'm struggling to find a better name - at least the WorkerSafe suffix is accurate. If you call it with data that was ok to use, it won't reference extra data that isn't. > > Source/WebCore/css/FontFace.cpp:84 > > - auto value = FontFace::parseString(string, CSSPropertySrc); > > - if (!is<CSSValueList>(value)) > > - return Exception { SyntaxError }; > > - CSSFontFace::appendSources(result->backing(), downcast<CSSValueList>(*value), &document, false); > > - return { }; > > + if (auto value = CSSPropertyParserWorkerSafe::parseFontFaceSrc(string, CSSParserContext(document))) { > > + CSSFontFace::appendSources(result->backing(), *value, &document, false); > > + return { }; > > + } > > + return Exception { SyntaxError }; > > Better: Guarding a local variable with a if. > Worse: Making the failure case the "main code" and the success case the > guarded block. There’s a clarity reason we prefer early return; too bad it’s > incompatible with guarding locals with if. Yeah, looking at this again, that's not nice - I'll revert this to how it was previously. > The call to CSSPropertyParserWorkerSafe::parseFontFaceSrc is so much more > verbose than the FontFace::parseString call with a really long class name. > And also, the need to explicitly write out CSSParserContext. Is there some > way we can keep this idiom a little simpler? I think this file references the namespace enough that it wouldn't be so bad to just stick a using namespace declaration and get rid of the constant 'CSSPropertyParserWorkerSafe' everywhere. It wouldn't hurt to add a variant of parseFontFaceSrc that just took a ScriptExecutionContext as the parameter, but maybe that can be shortened without adding an extra prototype... > > Source/WebCore/css/FontFace.cpp:178 > > + if (familyNameToUse.contains('\'') && is<Document>(context) && downcast<Document>(context).quirks().shouldStripQuotationMarkInFontFaceSetFamily()) > > Seems like code running in workers will need quirks too, eventually, for the > same reason we need them for documents. Probably should inherit them from > the document it’s created from. This code change here is a stopgap measure. Yes, though I suppose in an ideal future world full of workers and OffscreenCanvas, we wouldn't have quirks? One can dream :) > > Source/WebCore/css/FontFace.cpp:196 > > - if (auto value = parseString(style, CSSPropertyFontStyle)) { > > + if (auto value = CSSPropertyParserWorkerSafe::parseFontFaceStyle(style, HTMLStandardMode, context.cssValuePool())) { > > Frustrating that this code gets so verbose. Any way to cut down on the > arguments? Perhaps like with parseFontFaceSrc, these functions should all just take a ScriptExecutionContext, rather than try to pre-empt things and taking the only thing it actually needs. That, along with the namespace import would make this much shorter. > > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:50 > > +using namespace CSSPropertyParserHelpers; > > +using namespace CSSPropertyParserHelpersWorkerSafe; > > We should typically strive to avoid these kinds of "using namespace". > > I’m not sure what the namespaces achieve exactly, so I’m not sure what to > recommend. Hmm, now you say this I'm not sure about what I'm saying about namespace above... These can easily be removed (and the cost of some longer lines). > > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.cpp:186 > > +namespace CSSPropertyParserHelpersWorkerSafe { > > > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.h:60 > > +namespace CSSPropertyParserHelpersWorkerSafe { > > Not sure why we need a namespace here. Partially copying how things are in CSSPropertyParserHelpers, but also to make it clear that everything within it is 'WorkerSafe' and not have to suffix every function prototype. > > Source/WebCore/css/parser/CSSPropertyParserWorkerSafe.h:42 > > +namespace CSSPropertyParserHelpers { > > Not sure why this needs to go in a namespace. Whoops, this is just left-over from an earlier revision... > > Source/WebCore/dom/ScriptExecutionContext.cpp:240 > > +CSSValuePool& ScriptExecutionContext::cssValuePool() > > +{ > > + return CSSValuePool::singleton(); > > +} > > Maybe this should go in Document and it should be pure virtual in > ScriptExecutionContext? I wanted to avoid having to touch audio worklets (and any other future worker types I suppose) - Really, this is more specific to WindowOrWorkerGlobalScope than it is to ScriptExecutionContext I suppose, but we don't have that distinction at this level (that I'm aware of?) Having it in the base class felt a bit less wrong than having it in a sub-class where it would feel even more out-of-place. > > Source/WebCore/dom/ScriptExecutionContext.h:171 > > + virtual CSSValuePool& cssValuePool(); > > Not sure I fully understand why each script execution context has its own > CSS value pool. And I suppose this is the real root-cause of the above. This is really about Window and WorkerGlobalScope than it is about ScriptExecutionContext. The former two are both FontProviders, so they both have to be capable of parsing CSS font shorthands and FontFace properties. Within that context, having a CSS value pool makes sense, but it doesn't make perfect sense for a ScriptExecutionContext... I think solving this distinction is a bigger issue though. Everything I've not commented on I'll fix.

Created attachment 426088 [details] Patch

This patch addresses most comments, but doesn't address the "WorkerSafe" nomenclature or the perhaps questionable decision to put a CSSValuePool on ScriptExecutionContext - I think this is a reasonable stepping stone though so I hope taking the granted review and only addressing this much for now is ok. While ruminating over this, I think a lot of this vagary would be resolved by removing direct access to all singletons from the parsing code (which I think all hang off of CSSValuePool, but I might be misremembering). At that point, you could assume that all the CSS parsing code is 'worker safe' and it wouldn't need to be specifically singled out. That's not to say that the main thread couldn't use singletons, but that they ought to be accessible via a context object (CSSValuePool?) that can be replaced when that wouldn't be a safe thing to do, and for it to be a required parameter so there's less chance of doing the wrong thing when modifying code/adding new code. There's also an odd split that seems arbitrary to me in the CSS parsing between parsing that happens without needing to explicitly create a parser object and parsing that does. I think that's also a source of some oddness, but I haven't read deeply into the code enough to see why this is.

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

<rdar://problem/76696278>