[webkit-reviews] review denied: [Bug 129041] [CSS Grid Layout] Update named <grid-line> syntax to the last version of the specs : [Attachment 226499] Getting back the 'maybe_space' that produced the conflict, but protect it with 'ifdef'.

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Mar 13 11:36:04 PDT 2014


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

Attachment 226499: Getting back the 'maybe_space' that produced the conflict,
but protect it with 'ifdef'.
https://bugs.webkit.org/attachment.cgi?id=226499&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.


More information about the webkit-reviews mailing list