[webkit-reviews] review granted: [Bug 224749] Don't use the full CSS parser for CSSFontFaceSet : [Attachment 426410] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Chris Lord <clord at igalia.com>'s
request for review:
Bug 224749: Don't use the full CSS parser for CSSFontFaceSet
https://bugs.webkit.org/show_bug.cgi?id=224749

Attachment 426410: Patch

https://bugs.webkit.org/attachment.cgi?id=426410&action=review




--- Comment #2 from Darin Adler <darin at apple.com> ---
Comment on attachment 426410
  --> https://bugs.webkit.org/attachment.cgi?id=426410
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().fon
tGenericFamilies.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.