[webkit-reviews] review denied: [Bug 129041] [CSS Grid Layout] Update named grid lines syntax to the last version of the specs : [Attachment 225833] Patch rebased.

[bugzilla-daemon at webkit.org]

Sergio Villar Senin <svillar at igalia.com> has denied Javier Fernandez
<jfernandez at igalia.com>'s request for review:
Bug 129041: [CSS Grid Layout] Update named grid lines syntax to the last
version of the specs
https://bugs.webkit.org/show_bug.cgi?id=129041

Attachment 225833: Patch rebased.
https://bugs.webkit.org/attachment.cgi?id=225833&action=review

------- Additional Comments from Sergio Villar Senin <svillar at igalia.com>
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.