[Webkit-unassigned] [Bug 129041] [CSS Grid Layout] Update named grid lines syntax to the last version of the specs
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 5 01:28:03 PST 2014
https://bugs.webkit.org/show_bug.cgi?id=129041
Sergio Villar Senin <svillar at igalia.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #225833|review? |review-
Flag| |
--- Comment #6 from Sergio Villar Senin <svillar at igalia.com> 2014-03-05 01:25:05 PST ---
(From update of attachment 225833)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list