|
Description
Javier Fernandez
2014-02-19 06:17:48 PST
This is basically porting the patch from the following blink issue: * https://codereview.chromium.org/23528004 This patch seems to be necessary as well. Not sure whether it deserves its own bug, though. * https://chromiumcodereview.appspot.com/22929020 (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. Created attachment 225827 [details]
Patch
Created attachment 225833 [details]
Patch rebased.
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. Created attachment 226047 [details]
Applied suggested changes.
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.
(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. Created attachment 226127 [details]
Added gmake and xcode info of the new files.
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.
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 Created attachment 226297 [details]
Appplied suggested changes.
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.
Created attachment 226418 [details]
Removed the shift/reduce conflict from the trask_names_list syntax.
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.
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 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
Created attachment 226499 [details]
Getting back the 'maybe_space' that produced the conflict, but protect it with 'ifdef'.
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.
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. Created attachment 226681 [details]
Appplied suggested changes.
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.
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 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
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 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
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 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
Created attachment 226705 [details]
Rebased and fixed test failures.
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.
Comment on attachment 226705 [details]
Rebased and fixed test failures.
r=me
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 Created attachment 227558 [details]
Patch rebased.
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.
Created attachment 227559 [details]
Changelog updated.
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.
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 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
Created attachment 227609 [details]
Patch
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.
Comment on attachment 227609 [details] Patch Clearing flags on attachment: 227609 Committed r166157: <http://trac.webkit.org/changeset/166157> All reviewed patches have been landed. Closing bug. |