[Webkit-unassigned] [Bug 129041] [CSS Grid Layout] Update named grid lines syntax to the last version of the specs

[bugzilla-daemon at webkit.org]

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.