Bug 103313

Summary: [CSS Grid Layout] Implement support for grid-template
Product: WebKit Reporter: Julien Chaffraix <jchaffraix>
Component: Layout and RenderingAssignee: Sergio Villar Senin <svillar>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, buildbot, commit-queue, dino, donggwan.kim, eflews.bot, eric.carlson, esprehn+autocc, glenn, gyuyoung.kim, gyuyoung.kim, jer.noble, kling, kondapallykalyan, macpherson, menard, ojan, pnormand, rakuco, rniwa, svillar, syoichi, tony, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 103334    
Bug Blocks: 60731, 120044, 120045    
Attachments:
Description Flags
Patch
none
Patch
none
Patch dino: review+

Description Julien Chaffraix 2012-11-26 16:05:55 PST
grid-template allows the grid to be defined graphically using identifiers to name grid areas:

grid-template = [ " [ <identifier> | . ]+ " ]+ | none

We also need to be careful with the interaction with grid-definition-{rows|columns}.
Comment 1 Sergio Villar Senin 2013-10-08 08:42:29 PDT
Fixing this involves merging 3 different CLs from Blink:

*   Implement 'grid-template' parsing

    This change makes us recognize grid-template, parse it, store it and
    return properly to JavaScript.

    The specification mandates to validate to check that the areas defined
    by 'grid-template' are rectangular which involves a lot of the
    CSSParser code.

    Review URL: https://chromiumcodereview.appspot.com/18532004

*  [CSS Grid Layout] Fix grid-template serialization

    This is a follow-up of https://codereview.chromium.org/18532004/#msg6

    We didn't know if serializing strings was specified and it turns out
    that it is: http://dev.w3.org/csswg/cssom/#serialize-a-string


*  Move GridCoordinate and GridSpan to a separate file

    In order to implement 'grid-template', we need a way to describe a
    grid area's coordinates. That's what GridCoordinate is about and
    instead of duplicating code, splitting the classes will enable code
    sharing.

    While moving the code, added some comments about the 2 classes.

    Review URL: https://chromiumcodereview.appspot.com/18345011
Comment 2 Sergio Villar Senin 2013-10-08 10:10:22 PDT
Created attachment 213696 [details]
Patch
Comment 3 Sergio Villar Senin 2013-10-08 10:34:12 PDT
Created attachment 213699 [details]
Patch
Comment 4 Build Bot 2013-10-08 11:02:25 PDT
Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3758003
Comment 5 EFL EWS Bot 2013-10-08 11:06:47 PDT
Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3758006
Comment 6 Build Bot 2013-10-08 11:07:49 PDT
Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3714254
Comment 7 EFL EWS Bot 2013-10-08 11:09:56 PDT
Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3716246
Comment 8 Dean Jackson 2013-10-08 15:42:01 PDT
Comment on attachment 213699 [details]
Patch

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

Nearly ready once build errors are fixed.

> Source/WebCore/css/CSSGridTemplateValue.cpp:54
> +    NamedGridAreaMap::const_iterator end = gridAreaMap.end();
> +    for (NamedGridAreaMap::const_iterator it = gridAreaMap.begin(); it != end; ++it) {

You can use "auto" here now. for (auto it = gridAreaMap.begin(), end = gridAreaMap.end(); it != end; ++it)

> Source/WebCore/css/CSSGridTemplateValue.cpp:61
> +    end = gridAreaMap.end();
> +    for (NamedGridAreaMap::const_iterator it = gridAreaMap.begin(); it != end; ++it) {

Ditto.

> Source/WebCore/css/CSSParser.cpp:5173
> +        if (!columnCount) {
> +            columnCount = columnNames.size();
> +            ASSERT(columnCount);
> +        } else if (columnCount != columnNames.size()) {
> +            // The declaration is invalid is all the rows don't have the number of columns.
> +            return 0;
> +        }

Please write this with the return first.

if (columnCount != columnNames.size()) {
  // Comment, note typo is -> if
  return 0;
}

Although I guess that assumes that columnCount is non-zero too.

> Source/WebCore/css/CSSParser.cpp:5189
> +            NamedGridAreaMap::iterator gridAreaIt = gridAreaMap.find(gridAreaName);

Maybe auto gridAreaIterator?
Comment 9 Sergio Villar Senin 2013-10-09 00:37:44 PDT
Comment on attachment 213699 [details]
Patch

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

Thx for the review dino, new patch upcoming

> Source/WebCore/css/CSSGridTemplateValue.h:72
> +

Actually I'm also removing these three and using CSS_VALUE_TYPE_CASTS that does the same (and more) and it's more WebKit-style
Comment 10 Sergio Villar Senin 2013-10-09 00:51:32 PDT
Created attachment 213758 [details]
Patch
Comment 11 Dean Jackson 2013-10-09 15:31:42 PDT
Comment on attachment 213758 [details]
Patch

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

> Source/WebCore/css/CSSGridTemplateValue.cpp:2
> + * Copyright (C) 2013 Google Inc. All rights reserved.

Do you really mean to assign Google the copyright? Why not your own name? (do you work for them?)

> Source/WebCore/css/CSSGridTemplateValue.cpp:14
> + *     * Neither the name of Google Inc. nor the names of its

DItto.

> Source/WebCore/css/CSSGridTemplateValue.h:2
> + * Copyright (C) 2013 Google Inc. All rights reserved.

Ditto. And other places too.

> Source/WebCore/css/CSSParser.cpp:5177
> +        for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) {

Could you expand Col to Column? I don't see the need to shorten (there are a few instances of this)

> LayoutTests/fast/css-grid-layout/grid-template-get-set.html:67
> +    shouldBeEqualToString("window.getComputedStyle(gridWithSpanningRowsDotTemplate).getPropertyValue('-webkit-grid-template')", '"span" "."')

Please extract window.getComputedStyle(gridWithSpanningRowsDotTemplate).getPropertyValue('-webkit-grid-template') into a helper function.

In fact, the function could wrap the getElementById + shouldBeEqualToString part as well, so it is just something like testGridTemplateValue("gridWithDefaultTemplate", "none");

> LayoutTests/fast/css-grid-layout/grid-template-get-set.html:76
> +    element.style.webkitGridTemplate = "'foobar'";
> +    shouldBeEqualToString("window.getComputedStyle(element).getPropertyValue('-webkit-grid-template')", '"foobar"')

Hmm... maybe not the getElementById part then, but at least it could take an element as a parameter.
Comment 12 Ryosuke Niwa 2013-10-09 16:40:19 PDT
We need to keep the Google copyright notice if the code was copied from Blink or if the author looked at Blink code to write this.
Comment 13 Sergio Villar Senin 2013-10-10 01:01:36 PDT
(In reply to comment #11)
> (From update of attachment 213758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213758&action=review
> 
> > Source/WebCore/css/CSSGridTemplateValue.cpp:2
> > + * Copyright (C) 2013 Google Inc. All rights reserved.
> 
> Do you really mean to assign Google the copyright? Why not your own name? (do you work for them?)

As Ryosuke mentions this work is based on some patches from Blink so it's fair/correct/legal to keep the copyright notice as is. I might add the Igalia copyright too.

> > Source/WebCore/css/CSSParser.cpp:5177
> > +        for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) {
> 
> Could you expand Col to Column? I don't see the need to shorten (there are a few instances of this)

Also expanded lookAheadCol -> lookAheadColumn
 
> > LayoutTests/fast/css-grid-layout/grid-template-get-set.html:76
> > +    element.style.webkitGridTemplate = "'foobar'";
> > +    shouldBeEqualToString("window.getComputedStyle(element).getPropertyValue('-webkit-grid-template')", '"foobar"')
> 
> Hmm... maybe not the getElementById part then, but at least it could take an element as a parameter.

Done
Comment 14 Sergio Villar Senin 2013-10-10 01:37:38 PDT
Committed r157211: <http://trac.webkit.org/changeset/157211>
Comment 15 Sergio Villar Senin 2013-11-06 08:56:28 PST
*** Bug 119922 has been marked as a duplicate of this bug. ***