Bug 224749

Created attachment 426410 [details] Patch

Comment on attachment 426410 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426410&action=review Some ideas about better naming and minor code style tweak thoughts. > Source/WebCore/ChangeLog:11 > + Replace use of the full CSS parser in CSSFontFaceSet with > + CSSPropertyParserWorkerSafe::parseFont to parse font shorthands. This > + makes CSSFontFaceSet safe to use in a Worker (required for > + OffscreenCanvas) and ought also to be faster. You describe two obvious advantages here. Are there any disadvantages? > Source/WebCore/css/CSSFontFaceSet.cpp:311 > +static FontSelectionRequest computeFontSelectionRequest(CSSPropertyParserHelpers::FontRaw& fontRaw) Why can’t we just name this "font"? Do we really need raw in the variable name? Variable names don}t always have to repeat their type. > Source/WebCore/css/CSSFontFaceSet.cpp:314 > + ? WTF::switchOn(*fontRaw.weight, [&] (CSSValueID ident) { How about a word rather than "ident"? Below I see you using "valueID". Maybe "keyword" or "identifier"? Or just "weight" would be OK. > Source/WebCore/css/CSSFontFaceSet.cpp:333 > + auto stretchSelectionValue = *fontStretchValue(fontRaw.stretch ? *fontRaw.stretch : CSSValueNormal); Can we use valueOr here instead of ?: in the function. Glad you wrote a comment explaining why we know this won’t return null; would be even better if we found a way to sidestep that. > Source/WebCore/css/CSSFontFaceSet.cpp:335 > + auto valueID = fontRaw.style ? fontRaw.style->style : CSSValueNormal; I might name this local variable "style" or "styleKeyword". Naming it "valueID" seems a bit oblque. > Source/WebCore/css/CSSFontFaceSet.cpp:344 > + degrees = static_cast<float>(CSSPrimitiveValue::computeDegrees(fontRaw.style->angle->type, fontRaw.style->angle->value)); Do we really need to downcast to float? Can we keep it double until we construct the FontSelectionValue? Seems like FontSelectionValue should have a constructor that takes a double as well as the one that takes a float. > Source/WebCore/css/CSSFontFaceSet.cpp:346 > + else > + degrees = 0; I think initializing to 0 would be better than using else. > Source/WebCore/css/CSSFontFaceSet.cpp:372 > + auto fontRaw = CSSPropertyParserWorkerSafe::parseFont(font, HTMLStandardMode); Seems to me that the fontRaw deserves the name "font", and the font string passed in is the thing that should have a different name to make it clear what kind of unparsed font string it is. I think it’s really strange that we have two strings, one named font and one named string, coming into this function. Very hard to me to understand the purpose of the second string. > Source/WebCore/css/CSSFontFaceSet.cpp:378 > + for (auto& item : fontRaw->family) { Not thrilled about the name "item" for the family here. > Source/WebCore/css/CSSFontFaceSet.cpp:381 > + WTF::switchOn(item, [&] (CSSValueID ident) { Same comment about "ident" as above. Maybe familyKeyword. > Source/WebCore/css/CSSFontFaceSet.cpp:382 > + isGenericFamily = ident != CSSValueWebkitBody; Since isGenericFamily is initialized to false above, I suggest we put this expression inside the if and just set isGenericFamily to true in the one case. > Source/WebCore/css/CSSFontFaceSet.cpp:387 > + family = AtomString(m_owningFontSelector->scriptExecutionContext()->settingsValues().fontGenericFamilies.standardFontFamily()); Why is the explicit cast to AtomString needed here (and not below for familyString for example). > Source/WebCore/css/CSSFontFaceSet.cpp:390 > + }, [&] (const String& familyString) { > + family = familyString; It’s not great to have both family and familyString and the difference between them being so subtle. We can name these things whatever we want, and we should name them to help understand the difference between them. Maybe just rename to familyNonAtomString.

Created attachment 426526 [details] Patch

Most comments addressed in the patch, notes in-line: (In reply to Darin Adler from comment #2) > > Source/WebCore/ChangeLog:11 > > + Replace use of the full CSS parser in CSSFontFaceSet with > > + CSSPropertyParserWorkerSafe::parseFont to parse font shorthands. This > > + makes CSSFontFaceSet safe to use in a Worker (required for > > + OffscreenCanvas) and ought also to be faster. > > You describe two obvious advantages here. Are there any disadvantages? I added a note to say that it comes at a slight increase in code-length (although really this is only because we can't use Style::BuilderConverter). I don't think there are any disadvantages for this use-case, much like with canvas - this path has no need for the parsed CSS values, and that's the only drawback of using the raw path (that it doesn't give you CSS values, and generating them from the raw path doesn't necessarily give you the same result 1:1 as if you used the full parser). > Glad you wrote a comment explaining why we know this won’t return null; > would be even better if we found a way to sidestep that. I replaced the straight dereference with an assert and a later dereference, just in case, and eased up the wording of the comment - if someone changed either of the dependent functions independently, there's a chance this could fail, so now there's an assert next to the comment that explains why that happens. Tests should catch if someone ever did that too, these paths are used quite heavily, but better safe than sorry. > > Source/WebCore/css/CSSFontFaceSet.cpp:344 > > + degrees = static_cast<float>(CSSPrimitiveValue::computeDegrees(fontRaw.style->angle->type, fontRaw.style->angle->value)); > > Do we really need to downcast to float? Can we keep it double until we > construct the FontSelectionValue? Seems like FontSelectionValue should have > a constructor that takes a double as well as the one that takes a float. FontSelectionValue only has float constructors - I've left this as is for now. It's a 16-bit fixed point value, but I suppose there may be a range of values that double could express that float can't and may be closer to the nearest fixed point representation? I've not thought this through rigorously, but I don't expect there's much of a trade-off here and this apes the behaviour before this patch anyway.

(In reply to Chris Lord from comment #4) > (In reply to Darin Adler from comment #2) > > Do we really need to downcast to float? Can we keep it double until we > > construct the FontSelectionValue? Seems like FontSelectionValue should have > > a constructor that takes a double as well as the one that takes a float. > > FontSelectionValue only has float constructors - I've left this as is for > now. It's a 16-bit fixed point value, but I suppose there may be a range of > values that double could express that float can't and may be closer to the > nearest fixed point representation? I've not thought this through > rigorously, but I don't expect there's much of a trade-off here and this > apes the behaviour before this patch anyway. Oh, I missed this comment somehow from FontSelectionValue: > // Since floats have 23 mantissa bits, every value can be represented losslessly. Perhaps it would've been nicer to keep the double and cast only when constructing, but this is pretty minor I suppose.

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

(In reply to Chris Lord from comment #4) > generating them from the raw path doesn't necessarily give you the same > result 1:1 as if you used the full parser I’d like to know more about this. If it doesn’t guarantee the same result, what can be different?

Comment on attachment 426526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=426526&action=review > Source/WebCore/css/CSSFontFaceSet.cpp:379 > + WTF::switchOn(familyRaw, [&] (CSSValueID ident) { Loved the names in your final patch. Missed one here, though. This "ident" should be "keyword" or "familyKeyword".

(In reply to Darin Adler from comment #7) > (In reply to Chris Lord from comment #4) > > generating them from the raw path doesn't necessarily give you the same > > result 1:1 as if you used the full parser > > I’d like to know more about this. If it doesn’t guarantee the same result, > what can be different? For example, if you use CSS calc, going through this path would not allow you to reconstruct a calc string because it just gets resolved, it doesn't keep the data necessary to reconstruct the full expression (which using the full parser would). Similarly, if "SANS 10px" was equivalent to "sans 10px", using the full parser would let you retrieve the exact expression, where as if you reconstructed it using data from FontRaw, likelihood is you'd end up with only the final resolved value and not the valid original input if it differs. Another example, I had another implementation of the patch in bug 224426 that used the raw parsers instead of making the CSSValue-producing consume functions worker-safe which worked in principle, but failed tests because they (correctly) expect to be able to read back the style string in certain cases. (This should have been obvious, I got ahead of myself and didn't realise that style strings were read back in that case until I was half-way through) As we don't need to read back any style properties from FontFaceSet, we're ok to use the raw parser. I feel ok without adding a clarifying comment about this in this patch as if we did need to do that in the future (I'm not sure how or why it could change in this particular case, but for the sake of argument), FontRaw doesn't have any methods that you could use that would get you an almost-correct result, it'd be reasonably obvious that you'd need to use the full parser to do so.

(In reply to Darin Adler from comment #8) > Comment on attachment 426526 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=426526&action=review > > > Source/WebCore/css/CSSFontFaceSet.cpp:379 > > + WTF::switchOn(familyRaw, [&] (CSSValueID ident) { > > Loved the names in your final patch. Missed one here, though. This "ident" > should be "keyword" or "familyKeyword". doh! I'll fix this in a related patch given how small an issue it is.

(In reply to Chris Lord from comment #9) > For example, if you use CSS calc, going through this path would not allow > you to reconstruct a calc string because it just gets resolved, it doesn't > keep the data necessary to reconstruct the full expression (which using the > full parser would). I don’t want us to add a ton of huge comments. But I do want this kind of thing to be immediately obvious when reading the code. I’ll think about what to do.