[webkit-reviews] review granted: [Bug 197656] Implement page-break-* and -webkit-column-break-* as legacy-shorthands. : [Attachment 369686] Handle CSS generic keyword correctly

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Joonghun Park
<jh718.park at samsung.com>'s request for review:
Bug 197656: Implement page-break-* and -webkit-column-break-* as
legacy-shorthands.
https://bugs.webkit.org/show_bug.cgi?id=197656

Attachment 369686: Handle CSS generic keyword correctly

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




--- Comment #27 from Darin Adler <darin at apple.com> ---
Comment on attachment 369686
  --> https://bugs.webkit.org/attachment.cgi?id=369686
Handle CSS generic keyword correctly

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

> Source/WebCore/css/StyleProperties.cpp:728
> +    // FIXME: If all longhands are the same css-generic keyword(e.g. initial
or inherit),
> +    // then the shorthand should be serialized to that keyword.
> +    // It seems to be needed to handle this in a single function commonly
for all the shorthands,
> +    // not in each of the shorthand serialization function.

I think this is a good comment, but doesn’t belong here inside one of the
specific property value functions. The right place to put it is in
StyleProperties::getPropertyValue, I think.

If we had a FIXME here it would say:

    // FIXME: Remove this isGlobalKeyword check after we do this consistently
for all shorthands in getPropertyValue.

In fact, I think we could probably fix this in the same place we currently have
the isPendingSubstitutionValue check. If the web platform tests already have
sufficient coverage then I think we should come back and deal with this as soon
as possible. But I think we need to test this for every shorthand, because
while it seems logical that it’s correct for all of them, I am not certain that
it is, and a test case would be the best way to prove it to ourselves.

> Source/WebCore/css/StyleProperties.cpp:733
> +	   return "always";

This could be "always"_s for slightly better performance. It’s kind of
inelegant that this is the only hard-coded string in this entire source file,
by the way.

> Source/WebCore/css/StyleProperties.cpp:737
> +    if (valueId == CSSValueAuto || valueId == CSSValueAvoid || valueId ==
CSSValueLeft
> +	   || valueId == CSSValueRight)
> +	   return value->cssText();
> +    return String();

Why do we need to check these? There’s no reason we have to support invalid
values. I think we can just return value->cssText() for any case other than
CSSValuePage without checking all these specific cases. If we do want to check
them then I suggest a switch rather than an if statement.