[webkit-reviews] review granted: [Bug 224574] [css-display-3] Implement CSS Display Two Value Notation : [Attachment 426470] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Tim Nguyen (:ntim) <ntim at apple.com>'s
request for review:
Bug 224574: [css-display-3] Implement CSS Display Two Value Notation
https://bugs.webkit.org/show_bug.cgi?id=224574

Attachment 426470: Patch

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




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

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

> Source/WebCore/css/parser/CSSPropertyParser.cpp:385
> +    // Parse single keyword values

I think this would read better as a separate function:
consumeSingleKeywordDisplayValue perhaps.

Then the early return here could be scoped inside the if:

    if (auto keyword = consumeSingleKeywordDisplayValue(range))
	return keyword;

> Source/WebCore/css/parser/CSSPropertyParser.cpp:451
> +    displayOutside = displayOutside.valueOr(CSSValueBlock);
> +    displayInside = displayInside.valueOr(CSSValueFlow);

If these went into new locals instead of being modified in place then they
would not be optional any more which could get rid of the need for the * below.
Could name the ones inside the loop "parsedDisplayOutside" and then use new
locals here for the "valueOr" results.

> Source/WebCore/css/parser/CSSPropertyParser.cpp:453
> +    auto longValuesToShortValue = [](auto displayOutside, auto
displayInside) {

Not sure about capturing vs. arguments for this lambda, I think it should be
called just selectShortValue or shortValue, with no arguments. The only reason
it exists is so we can use "return". Above we use local variables instead of
return statements and a block. Could do it either way.

>
LayoutTests/imported/w3c/web-platform-tests/html/rendering/widgets/button-layou
t/computed-style.html:31
> +	 if (val == 'flow') {
> +	   // Use the more backwards-compatible form, `block` is better than
`flow`
> +	   // https://drafts.csswg.org/cssom/#serializing-css-values
> +	   expectedVal = 'block';
> +	 }

Is this change also being done upstream?