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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=129041


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #226499|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #21 from Darin Adler <darin at apple.com>  2014-03-13 11:36:28 PST ---
(From update of attachment 226499)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.