[webkit-reviews] review granted: [Bug 226770] CSSOM test for serializing font-variant fails : [Attachment 431793] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Rob Buis <rbuis at igalia.com>'s request
for review:
Bug 226770: CSSOM test for serializing font-variant fails
https://bugs.webkit.org/show_bug.cgi?id=226770

Attachment 431793: Patch

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




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 431793
  --> https://bugs.webkit.org/attachment.cgi?id=431793
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=431793&action=review

> Source/WebCore/css/parser/CSSPropertyParser.cpp:758
> +    if (variant) {
> +	   switch (*variant) {

Could write this instead:

    switch (variant.value_or(FontVariantEastAsianVariant::Normal)) {

Then we don’t have to nest the entire switch statement inside an if statement.
It’s also such a mess how these functions map an enumeration value to a CSS
value but don’t share any of the code to create the identifier and append. We
could make this a lot tidier with a bit of refactoring.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:4969
> +	   addProperty(CSSPropertyFontVariantCaps, CSSPropertyFontVariant,
CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important,
true);
> +	   addProperty(CSSPropertyFontVariantEastAsian, CSSPropertyFontVariant,
CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important,
true);
> +	   addProperty(CSSPropertyFontVariantPosition, CSSPropertyFontVariant,
CSSValuePool::singleton().createIdentifierValue(CSSValueNormal), important,
true);

This ", true" stuff is unreadable. This is why we don}t use booleans for
arguments where we pass constants.