[webkit-reviews] review denied: [Bug 70107] Custom written CSS lexer : [Attachment 121239] release patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Zoltan Herczeg
<zherczeg at webkit.org>'s request for review:
Bug 70107: Custom written CSS lexer
https://bugs.webkit.org/show_bug.cgi?id=70107

Attachment 121239: release patch
https://bugs.webkit.org/attachment.cgi?id=121239&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=121239&action=review


Okay, i'm doing a preliminary r- due to the large number of unnecessary
formatting changes sorry.  A patch of this scale and complexity shouldn't have
unnecessary no-op changes in it :-/

I'll continue with actual content review, but this should let you start working
on the formatting stuff without waiting for me to finish this review

> Source/WebCore/css/CSSParser.cpp:263
> -    int length = string.length() + strlen(prefix) + strlen(suffix) + 2;
> +    int length = string.length() + strlen(prefix) + strlen(suffix) + 1;

Why have we dropped to only a single extra null terminator?  I recall (though
haven't got that far in the patch) that we rely on two nulls at the end of the
string

> Source/WebCore/css/CSSParser.cpp:925
> -	   if (id == CSSValueStatic ||
> -		id == CSSValueRelative ||
> -		id == CSSValueAbsolute ||
> -		id == CSSValueFixed)
> +	   if (id == CSSValueStatic
> +	       || id == CSSValueRelative
> +	       || id == CSSValueAbsolute
> +	       || id == CSSValueFixed)

This change seems unnecessary formatting which i'd rather avoid in a patch of
this size and complexity

> Source/WebCore/css/CSSParser.cpp:939
> -	   if (id == CSSValueAuto ||
> -		id == CSSValueAlways ||
> -		id == CSSValueAvoid ||
> -		id == CSSValueLeft ||
> -		id == CSSValueRight)
> +	   if (id == CSSValueAuto
> +	       || id == CSSValueAlways
> +	       || id == CSSValueAvoid
> +	       || id == CSSValueLeft
> +	       || id == CSSValueRight)

ditto

> Source/WebCore/css/CSSParser.cpp:952
> -	   if (id == CSSValueShow ||
> -		id == CSSValueHide)
> +	   if (id == CSSValueShow
> +	       || id == CSSValueHide)

ditto

> Source/WebCore/css/CSSParser.cpp:965
> -	   if (id == CSSValueNormal ||
> -	       id == CSSValuePre ||
> -	       id == CSSValuePreWrap ||
> -	       id == CSSValuePreLine ||
> -	       id == CSSValueNowrap)
> +	   if (id == CSSValueNormal
> +	       || id == CSSValuePre
> +	       || id == CSSValuePreWrap
> +	       || id == CSSValuePreLine
> +	       || id == CSSValueNowrap)

ditto

>> Source/WebCore/css/CSSParser.cpp:1424
>> -	 case CSSPropertyCounterReset:	      // [ <identifier> <integer>? ]+ |
none | inherit
>> +	case CSSPropertyCounterReset:	     // [ <identifier> <integer>? ]+ |
none | inherit
> 
> One space before end of line comments  [whitespace/comments] [5]

ditto

> Source/WebCore/css/CSSParser.cpp:2025
> -		validPrimitive = true;
> +	       validPrimitive = true;

ditto

> Source/WebCore/css/CSSParser.cpp:2894
> -	  return cssValuePool()->createIdentifierValue(id);
> +	   return cssValuePool()->createIdentifierValue(id);

ditto

> Source/WebCore/css/CSSParser.cpp:5186
> -				 // it is a color, but a color isn't allowed at
this point.
> +			     // it is a color, but a color isn't allowed at
this point.

ditto

> Source/WebCore/css/CSSParser.cpp:5474
> -	return id == CSSValueStretch || id == CSSValueRepeat || id ==
CSSValueSpace || id == CSSValueRound;
> +    return id == CSSValueStretch || id == CSSValueRepeat || id ==
CSSValueSpace || id == CSSValueRound;

ditto

> Source/WebCore/css/CSSParser.cpp:5559
> -		m_left = m_cssValuePool->createValue(m_right->getDoubleValue(),
(CSSPrimitiveValue::UnitTypes)m_right->primitiveType());
> +	       m_left = m_cssValuePool->createValue(m_right->getDoubleValue(),
(CSSPrimitiveValue::UnitTypes)m_right->primitiveType());

ditto