Bug 134057 – [CSS Grid Layout] Update grid-auto-flow to the new syntax

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Manuel Rego Casasnovas]

grid-auto-flow syntax has changed in the last versions of the spec.

This is being addressed in Blink (https://codereview.chromium.org/333563003/) and should be ported to WebKit.

[Manuel Rego Casasnovas]

Created attachment 234248 [details]
Patch

[Build Bot]

Comment on attachment 234248 [details]
Patch

Attachment 234248 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5125708200280064

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 234256 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 234248 [details]
Patch

Attachment 234248 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4803379494649856

New failing tests:
svg/css/getComputedStyle-basic.xhtml

[Build Bot]

Created attachment 234257 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Manuel Rego Casasnovas]

Created attachment 234259 [details]
Rebaseline for svg/css/getComputedStyle-basic.html.

[Sergio Villar Senin]

Comment on attachment 234259 [details]
Rebaseline for svg/css/getComputedStyle-basic.html.

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?

[Manuel Rego Casasnovas]

Created attachment 234706 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to comment #7)
> (From update of attachment 234259 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234259&action=review
> 
> Promising, but I think we need some adjustments before landing.

Thanks for the detailed review, I've just uploaded a new version applying suggested changes.

[Sergio Villar Senin]

Comment on attachment 234706 [details]
Patch

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

Awesome! Thanks for the patch.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2057
> +            else if (style->isGridAutoFlowDirectionColumn())

Is there any other possibility? I mean the direction is either row or column so we can use a simple "else". If we want to be ultra-sure we could always add an ASSERT(isColumn() || isRow()).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2063
> +                list->append(cssValuePool().createIdentifierValue(CSSValueStack));

Same comment here.

> Source/WebCore/css/CSSParser.cpp:5191
> +    // Second parameter, if any.

We must improve this comment mentioning that this can be used by the grid shorthand. It isn't obvious from the code why we don't bail out in some, apparently erroneous cases.

[Manuel Rego Casasnovas]

Comment on attachment 234706 [details]
Patch

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

Thanks for the review

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2057
>> +            else if (style->isGridAutoFlowDirectionColumn())
> 
> Is there any other possibility? I mean the direction is either row or column so we can use a simple "else". If we want to be ultra-sure we could always add an ASSERT(isColumn() || isRow()).

No, you're right the only options are row or column (I'm removing the second if and adding the ASSERT).

>> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:2063
>> +                list->append(cssValuePool().createIdentifierValue(CSSValueStack));
> 
> Same comment here.

In this case there's one extra option isGridAutoFlowAlgorithmSparse(), so I'll keep it as it's.

>> Source/WebCore/css/CSSParser.cpp:5191
>> +    // Second parameter, if any.
> 
> We must improve this comment mentioning that this can be used by the grid shorthand. It isn't obvious from the code why we don't bail out in some, apparently erroneous cases.

Added explicit comment about the shorthand.

[Manuel Rego Casasnovas]

Created attachment 234753 [details]
Patch for landing

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 234753 [details]:

media/video-ended-event-negative-playback.html bug 134490
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 234753 [details]
Patch for landing

Clearing flags on attachment: 234753

Committed r170996: <http://trac.webkit.org/changeset/170996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.