Bug 129041

Summary: [CSS Grid Layout] Update named <grid-line> syntax to the last version of the specs
Product: WebKit Reporter: Javier Fernandez <jfernandez>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: allan.jensen, buildbot, bunhere, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, jfernandez, kling, macpherson, menard, rakuco, rego, rniwa, sergio, svillar, syoichi, webkit-ews
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 127837    
Bug Blocks: 127987, 128980, 129713    
Attachments:
Description Flags
Patch
none
Patch rebased.
none
Applied suggested changes.
none
Added gmake and xcode info of the new files.
none
Appplied suggested changes.
none
Removed the shift/reduce conflict from the trask_names_list syntax.
none
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion
none
Getting back the 'maybe_space' that produced the conflict, but protect it with 'ifdef'.
none
Appplied suggested changes.
none
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2
none
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2
none
Rebased and fixed test failures.
none
Patch rebased.
none
Changelog updated.
none
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Patch none

Javier Fernandez
Reported 2014-02-19 06:17:48 PST
Names for grid lines are now specified as a list of whitespace separated idents enclosed in parentheses instead of as a list of strings.
Attachments
Patch (115.16 KB, patch)
2014-03-04 16:05 PST, Javier Fernandez
no flags
Patch rebased. (120.06 KB, patch)
2014-03-04 17:11 PST, Javier Fernandez
no flags
Applied suggested changes. (117.98 KB, patch)
2014-03-06 15:19 PST, Javier Fernandez
no flags
Added gmake and xcode info of the new files. (123.08 KB, patch)
2014-03-07 07:29 PST, Javier Fernandez
no flags
Appplied suggested changes. (122.54 KB, patch)
2014-03-10 04:47 PDT, Javier Fernandez
no flags
Removed the shift/reduce conflict from the trask_names_list syntax. (122.78 KB, patch)
2014-03-11 05:10 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-03 for mac-mountainlion (652.58 KB, application/zip)
2014-03-11 07:03 PDT, Build Bot
no flags
Getting back the 'maybe_space' that produced the conflict, but protect it with 'ifdef'. (122.87 KB, patch)
2014-03-12 06:21 PDT, Javier Fernandez
no flags
Appplied suggested changes. (123.81 KB, patch)
2014-03-14 04:27 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2 (458.77 KB, application/zip)
2014-03-14 05:40 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion (491.69 KB, application/zip)
2014-03-14 06:16 PDT, Build Bot
no flags
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (465.18 KB, application/zip)
2014-03-14 06:46 PDT, Build Bot
no flags
Rebased and fixed test failures. (124.17 KB, patch)
2014-03-14 06:52 PDT, Javier Fernandez
no flags
Patch rebased. (123.97 KB, patch)
2014-03-22 11:41 PDT, Javier Fernandez
no flags
Changelog updated. (123.97 KB, patch)
2014-03-22 11:51 PDT, Javier Fernandez
no flags
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (488.73 KB, application/zip)
2014-03-22 13:39 PDT, Build Bot
no flags
Patch (123.99 KB, patch)
2014-03-23 08:05 PDT, Javier Fernandez
no flags
Javier Fernandez
Comment 1 2014-02-19 07:37:46 PST
This is basically porting the patch from the following blink issue: * https://codereview.chromium.org/23528004
Javier Fernandez
Comment 2 2014-02-19 07:41:50 PST
This patch seems to be necessary as well. Not sure whether it deserves its own bug, though. * https://chromiumcodereview.appspot.com/22929020
Javier Fernandez
Comment 3 2014-02-20 14:02:56 PST
(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
Comment 4 2014-03-04 16:05:37 PST
Javier Fernandez
Comment 5 2014-03-04 17:11:55 PST
Created attachment 225833 [details] Patch rebased.
Sergio Villar Senin
Comment 6 2014-03-05 01:25:05 PST
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
Comment 7 2014-03-06 15:19:57 PST
Created attachment 226047 [details] Applied suggested changes.
WebKit Commit Bot
Comment 8 2014-03-06 15:21:32 PST
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
Comment 9 2014-03-06 15:22:23 PST
(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
Comment 10 2014-03-07 07:29:46 PST
Created attachment 226127 [details] Added gmake and xcode info of the new files.
WebKit Commit Bot
Comment 11 2014-03-07 07:31:33 PST
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 12 2014-03-07 08:31:01 PST
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
Comment 13 2014-03-10 04:47:08 PDT
Created attachment 226297 [details] Appplied suggested changes.
WebKit Commit Bot
Comment 14 2014-03-10 04:49:29 PDT
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
Comment 15 2014-03-11 05:10:17 PDT
Created attachment 226418 [details] Removed the shift/reduce conflict from the trask_names_list syntax.
WebKit Commit Bot
Comment 16 2014-03-11 05:11:46 PDT
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 17 2014-03-11 07:03:04 PDT
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
Comment 18 2014-03-11 07:03:12 PDT
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
Comment 19 2014-03-12 06:21:59 PDT
Created attachment 226499 [details] Getting back the 'maybe_space' that produced the conflict, but protect it with 'ifdef'.
Early Warning System Bot
Comment 20 2014-03-13 02:31:19 PDT
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 21 2014-03-13 11:36:28 PDT
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
Comment 22 2014-03-14 04:27:04 PDT
Created attachment 226681 [details] Appplied suggested changes.
WebKit Commit Bot
Comment 23 2014-03-14 04:28:30 PDT
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 24 2014-03-14 05:40:10 PDT
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
Comment 25 2014-03-14 05:40:16 PDT
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 26 2014-03-14 06:15:58 PDT
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
Comment 27 2014-03-14 06:16:06 PDT
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 28 2014-03-14 06:46:31 PDT
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
Comment 29 2014-03-14 06:46:37 PDT
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
Comment 30 2014-03-14 06:52:58 PDT
Created attachment 226705 [details] Rebased and fixed test failures.
WebKit Commit Bot
Comment 31 2014-03-14 06:55:34 PDT
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 32 2014-03-21 10:00:06 PDT
Comment on attachment 226705 [details] Rebased and fixed test failures. r=me
WebKit Commit Bot
Comment 33 2014-03-21 10:01:34 PDT
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
Comment 34 2014-03-22 11:41:51 PDT
Created attachment 227558 [details] Patch rebased.
WebKit Commit Bot
Comment 35 2014-03-22 11:49:18 PDT
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
Comment 36 2014-03-22 11:51:26 PDT
Created attachment 227559 [details] Changelog updated.
WebKit Commit Bot
Comment 37 2014-03-22 11:54:11 PDT
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 38 2014-03-22 13:39:08 PDT
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
Comment 39 2014-03-22 13:39:14 PDT
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
Comment 40 2014-03-23 08:05:58 PDT
WebKit Commit Bot
Comment 41 2014-03-23 08:09:33 PDT
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 42 2014-03-24 02:42:48 PDT
Comment on attachment 227609 [details] Patch Clearing flags on attachment: 227609 Committed r166157: <http://trac.webkit.org/changeset/166157>
WebKit Commit Bot
Comment 43 2014-03-24 02:42:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.