Bug 134057 - [CSS Grid Layout] Update grid-auto-flow to the new syntax
Summary: [CSS Grid Layout] Update grid-auto-flow to the new syntax
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 103316 134544
  Show dependency treegraph
 
Reported: 2014-06-19 02:44 PDT by Manuel Rego Casasnovas
Modified: 2014-07-11 03:43 PDT (History)
19 users (show)

See Also:


Attachments
Patch (103.74 KB, patch)
2014-07-02 03:30 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (538.93 KB, application/zip)
2014-07-02 05:28 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (501.45 KB, application/zip)
2014-07-02 05:49 PDT, Build Bot
no flags Details
Rebaseline for svg/css/getComputedStyle-basic.html. (105.73 KB, patch)
2014-07-02 06:08 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch (105.20 KB, patch)
2014-07-10 09:23 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (105.35 KB, patch)
2014-07-11 02:56 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Manuel Rego Casasnovas 2014-06-19 02:44:00 PDT
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.
Comment 1 Manuel Rego Casasnovas 2014-07-02 03:30:47 PDT
Created attachment 234248 [details]
Patch
Comment 2 Build Bot 2014-07-02 05:28:33 PDT
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
Comment 3 Build Bot 2014-07-02 05:28:38 PDT
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
Comment 4 Build Bot 2014-07-02 05:48:59 PDT
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
Comment 5 Build Bot 2014-07-02 05:49:08 PDT
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
Comment 6 Manuel Rego Casasnovas 2014-07-02 06:08:13 PDT
Created attachment 234259 [details]
Rebaseline for svg/css/getComputedStyle-basic.html.
Comment 7 Sergio Villar Senin 2014-07-09 02:25:16 PDT
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?
Comment 8 Manuel Rego Casasnovas 2014-07-10 09:23:46 PDT
Created attachment 234706 [details]
Patch
Comment 9 Manuel Rego Casasnovas 2014-07-10 09:25:16 PDT
(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.
Comment 10 Sergio Villar Senin 2014-07-11 01:39:48 PDT
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.
Comment 11 Manuel Rego Casasnovas 2014-07-11 02:46:09 PDT
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.
Comment 12 Manuel Rego Casasnovas 2014-07-11 02:56:17 PDT
Created attachment 234753 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2014-07-11 03:41:51 PDT
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.
Comment 14 WebKit Commit Bot 2014-07-11 03:42:56 PDT
Comment on attachment 234753 [details]
Patch for landing

Clearing flags on attachment: 234753

Committed r170996: <http://trac.webkit.org/changeset/170996>
Comment 15 WebKit Commit Bot 2014-07-11 03:43:03 PDT
All reviewed patches have been landed.  Closing bug.