[webkit-reviews] review denied: [Bug 134057] [CSS Grid Layout] Update grid-auto-flow to the new syntax : [Attachment 234259] Rebaseline for svg/css/getComputedStyle-basic.html.

[bugzilla-daemon at webkit.org]

Sergio Villar Senin <svillar at igalia.com> has denied Manuel Rego Casasnovas
<rego at igalia.com>'s request for review:
Bug 134057: [CSS Grid Layout] Update grid-auto-flow to the new syntax
https://bugs.webkit.org/show_bug.cgi?id=134057

Attachment 234259: Rebaseline for svg/css/getComputedStyle-basic.html.
https://bugs.webkit.org/attachment.cgi?id=234259&action=review

------- Additional Comments from Sergio Villar Senin <svillar at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=234259&action=review


Promising, but I think we need some adjustments before landing.

> Source/WebCore/ChangeLog:17
> +	   once "stack" is implemented.

Looks confusing. Please reword.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2089
> +	   }

I think we could simplify this a lot. You'll find the rationale in my last
comment in StyleResolver::applyProperty()

> Source/WebCore/css/CSSParser.cpp:5183
> +    if (value->id != CSSValueRow && value->id != CSSValueColumn && value->id
!= CSSValueDense && value->id != CSSValueStack)

Weird, you store value->id in firstId but you keep using value->id.

> Source/WebCore/css/CSSParser.cpp:5192
> +    if (value) {

Let's do an early return here and avoid an indentation level.

> Source/WebCore/css/CSSParser.cpp:5205
> +	       return parsedValues;

Hmm, firstId cannot be anything else right? We should have detected it in the
early return above so we should assert here.

> Source/WebCore/css/CSSParser.cpp:5211
> +    return parsedValues;

I have a comment about how we generate the list of parsedValues. Check my last
comment in StyleResolver::applyProperty()

> Source/WebCore/css/StyleResolver.cpp:2805
> +

I prefer something like:

if (!list->length()) {
    state.style()->setGridAutoFlow(RenderStyle::initialGridAutoFlow());
    return;
}
CSSPrimitiveValue* first = toCSSPrimitiveValue(list->item(0));

> Source/WebCore/css/StyleResolver.cpp:2815
> +		       autoFlow = AutoFlowStackRow;

autoFlow = second->getValueID() == CSSValueDense ? AutoFlowRowDense :
AutoFlowStackRow;

> Source/WebCore/css/StyleResolver.cpp:2824
> +		       autoFlow = AutoFlowStackColumn;

Same comment than above

> Source/WebCore/css/StyleResolver.cpp:2832
> +		   autoFlow = AutoFlowRowDense;

Again.

> Source/WebCore/css/StyleResolver.cpp:2838
> +		   autoFlow = AutoFlowStackRow;

And again.

> Source/WebCore/css/StyleResolver.cpp:2843
> +	   }

All those switch statements everywhere made me wonder. Although the css syntax
is 

[ row | column ] && dense? | stack && [ row | column ]?

we could get a more convenient representation from the parser. So after
checking (in the parser) that the syntax is correct we can generate a "fixed"
representation of the value list. For example, we can force that the parser
always generates a list of two elements, being the first one [column|row] and
the second one [dense|stack]. That will simplify a lot the code above removing
unnecessary checks and avoiding the need of checks for all possible
permutations.

> Source/WebCore/rendering/style/RenderStyle.h:933
> +    bool isGridAutoFlowAlgorithmStack() const { return
(rareNonInheritedData->m_grid->m_gridAutoFlow & InternalAutoFlowAlgorithmStack)
== InternalAutoFlowAlgorithmStack; }

Do we really need the "== Internal..." part in all these?