Bug 129041 – [CSS Grid Layout] Update named <grid-line> syntax to the last version of the specs

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Javier Fernandez]

Names for grid lines are now specified as a list of whitespace separated idents enclosed in parentheses instead of as a list of strings.

[Javier Fernandez]

This is basically porting the patch from the following blink issue:

 * https://codereview.chromium.org/23528004

[Javier Fernandez]

This patch seems to be necessary as well. Not sure whether it deserves its own bug, though. 

* https://chromiumcodereview.appspot.com/22929020

[Javier Fernandez]

(In reply to comment #2)
> This patch seems to be necessary as well. Not sure whether it deserves its own bug, though. 
> 
> * https://chromiumcodereview.appspot.com/22929020

There is already a bug for porting this patch, it's #127837, so added dependency.

[Javier Fernandez]

Created attachment 225827 [details]
Patch

[Javier Fernandez]

Created attachment 225833 [details]
Patch rebased.

[Sergio Villar Senin]

Comment on attachment 225833 [details]
Patch rebased.

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

Looks like the patch does not apply. As a general comment be careful of adding the code inside the compiler guards.

I'm setting the r- also because the issues in the grammar.

> Source/WebCore/ChangeLog:7
> +

You should probably mention the original author? ;). Ditto to the other ChangeLogs.

> Source/WebCore/css/CSSGrammar.y.in:71
> +inline static CSSParserValue makeIdentValue(CSSParserString string)

"static inline" is much more common.

> Source/WebCore/css/CSSGrammar.y.in:82
> +%expect 30

Hmm this is bad, are you sure that it adds reduce/shift conflicts? IIRC the patch that landed in Blink didn't add any.

> Source/WebCore/css/CSSGrammar.y.in:1400
> +

See my comment bellow about sinkFloatingValueList/createFloatingValueList.

> Source/WebCore/css/CSSParser.cpp:4878
> +void CSSParser::parseGridLineNames(CSSParserValueList* parserValues, CSSValueList& valueList)

Definitely not correct. You should keep using the reference as it cannot be null. Then fix the usage of parserValues inside the function.

> Source/WebCore/css/CSSParser.cpp:11393
> +

I think we shouldn't need these two. Check how we create CSSParserValueLists and handle them with std::unique_ptr directly in the grammar.

> Source/WebCore/css/CSSParser.h:166
> +    void parseGridLineNames(CSSParserValueList* inputList, CSSValueList&);

Reference not raw pointer.

> Source/WebCore/css/CSSValue.cpp:43
> +#include "CSSGridLineNamesValue.h"

Only when ENABLE(CSS_GRID_LAYOUT).

> Source/WebCore/css/CSSValue.h:106
> +    bool isGridLineNamesValue() const { return m_classType == GridLineNamesClass; }

This should go inside the ENABLE(CSS_GRID_LAYOUT) block.

> Source/WebCore/css/CSSValue.h:178
> +        GridLineNamesClass,

Ditto.

[Javier Fernandez]

Created attachment 226047 [details]
Applied suggested changes.

[WebKit Commit Bot]

Attachment 226047 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 36 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Javier Fernandez]

(In reply to comment #6)
> (From update of attachment 225833 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=225833&action=review
> 
> > Source/WebCore/css/CSSGrammar.y.in:82
> > +%expect 30
> 
> Hmm this is bad, are you sure that it adds reduce/shift conflicts? IIRC the patch that landed in Blink didn't add any.
> 

At least it fails at build time because if that, claiming that it expects 30 shift/reduce conflicts.

[Javier Fernandez]

Created attachment 226127 [details]
Added gmake and xcode info of the new files.

[WebKit Commit Bot]

Attachment 226127 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Comment on attachment 226127 [details]
Added gmake and xcode info of the new files.

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

Getting better, but r- due to missing guards and mistakes in the tests.

> Source/WebCore/ChangeLog:13
> +        Updated tests to match the new grid line-name syntax.

the new <grid-line>

> Source/WebCore/css/CSSGrammar.y.in:278
> +%type <value> track_names_list

Maybe we could guard the grammar code. The ident_list is general purpose and might be reused in the future by some other stuff but it would be a matter of moving it out of the guards.

> Source/WebCore/css/CSSParser.cpp:4894
> +        lineNames->append(lineName.release());

You can join these two in a single line.

> Source/WebCore/css/CSSParser.cpp:4932
> +        // This will handle the trailing <ident>* in the grammar.

<custom-ident>

> Source/WebCore/css/CSSParser.cpp:4957
> +    // Handle leading <ident>*.

Ditto.

> Source/WebCore/css/CSSParser.cpp:4973
> +        // This takes care of any trailing <ident>* in the grammar.

Ditto.

> Source/WebCore/css/CSSParser.h:570
> +    Vector<CSSParserValueList*> m_floatingValueLists;

This is not needed.

> Source/WebCore/css/CSSValue.cpp:43
> +#include "CSSGridLineNamesValue.h"

ENABLE(CSS_GRID_LAYOUT)

> Source/WebCore/css/StyleResolver.cpp:44
> +#include "CSSGridLineNamesValue.h"

ENABLE(CSS_GRID_LAYOUT)

> LayoutTests/fast/css-grid-layout/grid-item-negative-position-resolution.html:102
> +    -webkit-grid-row: 1;

Good catch!!! but you forgot the "middle".

> LayoutTests/fast/css-grid-layout/named-grid-line-get-set-expected.txt:67
> +FAIL getComputedStyle(element, '').getPropertyValue('-webkit-grid-template-columns') should be (first nav) 0px (nav) 0px (last). Was (nav first) 0px (nav) 0px (last).

Those FAIL's are not correct. The problem is that the line names are listed in the wrong order. We have bug 127837 for that. In the meantime just use the unordered version.

> LayoutTests/fast/css-grid-layout/non-named-grid-line-get-set-expected.txt:17
> +FAIL window.getComputedStyle(gridWithPercentageSameStringMultipleTimes, '').getPropertyValue('-webkit-grid-template-columns') should be (first nav) 10% (nav) 15% (last). Was (nav first) 10% (nav) 15% (last).

Same issue here with the order of the line names.

> LayoutTests/fast/css-grid-layout/non-named-grid-line-get-set-expected.txt:61
> +FAIL getComputedStyle(element, '').getPropertyValue('-webkit-grid-template-columns') should be (first nav) minmax(-webkit-min-content, -webkit-max-content) (nav) auto (last). Was (nav first) minmax(-webkit-min-content, -webkit-max-content) (nav) auto (last).

Ditto.

> LayoutTests/fast/css-grid-layout/non-named-grid-line-get-set.html:92
> +    element.style.webkitGridTemplateColumns = "(foo 'bar)";

extra "'" here

[Javier Fernandez]

Created attachment 226297 [details]
Appplied suggested changes.

[WebKit Commit Bot]

Attachment 226297 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Javier Fernandez]

Created attachment 226418 [details]
Removed the shift/reduce conflict from the trask_names_list syntax.

[WebKit Commit Bot]

Attachment 226418 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 226418 [details]
Removed the shift/reduce conflict from the trask_names_list syntax.

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

New failing tests:
fast/css-grid-layout/grid-columns-rows-get-set.html
fast/css-grid-layout/named-grid-lines-with-named-grid-areas-resolution.html
fast/css-grid-layout/grid-item-named-grid-line-resolution.html
fast/css-grid-layout/grid-item-bad-resolution-double-span.html
fast/css-grid-layout/grid-item-position-changed-dynamic.html
fast/css-grid-layout/grid-columns-rows-get-set-multiple.html
fast/css-grid-layout/grid-item-negative-position-resolution.html
fast/css-grid-layout/named-grid-line-get-set.html
fast/css-grid-layout/non-named-grid-line-get-set.html
fast/css-grid-layout/non-grid-columns-rows-get-set-multiple.html
fast/css-grid-layout/non-grid-element-repeat-get-set.html
fast/css-grid-layout/non-grid-columns-rows-get-set.html
fast/css-grid-layout/grid-item-named-grid-area-resolution.html
fast/css-grid-layout/grid-element-repeat-get-set.html

[Build Bot]

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

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

[Javier Fernandez]

Created attachment 226499 [details]
Getting back the 'maybe_space' that produced the conflict, but protect it with 'ifdef'.

[Early Warning System Bot]

Attachment 226499 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 226499 [details]
Getting back the 'maybe_space' that produced the conflict, but protect it with 'ifdef'.

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

Looks good. review- because of the storage leaks

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:40
> +#if ENABLE(CSS_GRID_LAYOUT)
> +#include "CSSGridLineNamesValue.h"
>  #include "CSSGridTemplateAreasValue.h"
> +#endif

Conditional includes go in a separate paragraph, not sorted in with the rest of the includes. Look at the sections for CSS_SHAPES and other defines in this same source file for an example of how to do it correctly.

> Source/WebCore/css/CSSGrammar.y.in:282
> +%type <valueList> ident_list

This will result in a storage leak in some parsing error cases. Any <valueList> element needs a %destructor. See the %type <valueList> section above for an example of how we do this.

> Source/WebCore/css/CSSGrammar.y.in:283
> +%type <value> track_names_list

This will result in a storage leak in some parsing error cases. The <value> element needs a %destructor. See the %type <value> section above for an example of how we do this.

> Source/WebCore/css/CSSGridLineNamesValue.h:58
> +CSS_VALUE_TYPE_CASTS(CSSGridLineNamesValue, isGridLineNamesValue());
> +}
> +
> +
> +#endif

Blank lines here are wrong. Should move the "}" down one line.

> Source/WebCore/css/CSSParser.cpp:43
> +#if ENABLE(CSS_GRID_LAYOUT)

Same incorrect includes formatting here. Conditional includes go into a separate paragraph.

> Source/WebCore/css/CSSParser.cpp:4963
> -        if (arguments->current()->unit == CSSPrimitiveValue::CSS_STRING)
> -            parseGridTrackNames(*arguments, *repeatedValues);
>  
>          if (!arguments->current())

Extra blank line left here. Please delete it.

> Source/WebCore/css/CSSParserValues.cpp:39
>  {
>      if (value.unit == CSSParserValue::Function)
>          delete value.function;
> +    else if (value.unit == CSSParserValue::ValueList)
> +        delete value.valueList;
>  }

Making this change means we have to call destroy on parser values even if we are not certain they are functions. This means we have to examine the cases in CSSParser.y where it says we don’t need to call destroy because the values are never functions and make sure they are never functions *or value lists*. And revise the comment that says, "These two parser values never need to be destroyed because they are never functions" to say "These two parser values never need to be destroyed because they are never functions or value lists".

> Source/WebCore/css/CSSParserValues.h:126
> +    inline void setFromValueList(std::unique_ptr<CSSParserValueList>);

I believe the "inline" here has no effect" and should be omitted.

If this is a good pattern I’d like to see it used for the other types too, not just the new one.

[Javier Fernandez]

Created attachment 226681 [details]
Appplied suggested changes.

[WebKit Commit Bot]

Attachment 226681 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 226681 [details]
Appplied suggested changes.

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

New failing tests:
fast/css-grid-layout/grid-item-named-grid-line-resolution.html

[Build Bot]

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

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

[Build Bot]

Comment on attachment 226681 [details]
Appplied suggested changes.

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

New failing tests:
fast/css-grid-layout/grid-item-named-grid-line-resolution.html

[Build Bot]

Created attachment 226698 [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 226681 [details]
Appplied suggested changes.

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

New failing tests:
fast/css-grid-layout/grid-item-named-grid-line-resolution.html

[Build Bot]

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

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

[Javier Fernandez]

Created attachment 226705 [details]
Rebased and fixed test failures.

[WebKit Commit Bot]

Attachment 226705 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergio Villar Senin]

Comment on attachment 226705 [details]
Rebased and fixed test failures.

r=me

[WebKit Commit Bot]

Comment on attachment 226705 [details]
Rebased and fixed test failures.

Rejecting attachment 226705 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 226705, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
s-grid-layout/resources/grid-columns-rows-get-set-multiple.js
patching file LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js
patching file LayoutTests/fast/css-grid-layout/resources/non-grid-columns-rows-get-set-multiple.js
patching file LayoutTests/fast/css-grid-layout/resources/non-grid-columns-rows-get-set.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Sergio Villar Senin']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/4850857027829760

[Javier Fernandez]

Created attachment 227558 [details]
Patch rebased.

[WebKit Commit Bot]

Attachment 227558 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Javier Fernandez]

Created attachment 227559 [details]
Changelog updated.

[WebKit Commit Bot]

Attachment 227559 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 227559 [details]
Changelog updated.

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

New failing tests:
compositing/columns/composited-rl-paginated-repaint.html

[Build Bot]

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

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

[Javier Fernandez]

Created attachment 227609 [details]
Patch

[WebKit Commit Bot]

Attachment 227609 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParserValues.h:122:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 1 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 227609 [details]
Patch

Clearing flags on attachment: 227609

Committed r166157: <http://trac.webkit.org/changeset/166157>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.