[webkit-reviews] review denied: [Bug 82056] Use enumeration for CSS parser mode : [Attachment 133571] Patch

[bugzilla-daemon at webkit.org]

Nikolas Zimmermann <zimmermann at kde.org> has denied Dirk Schulze
<krit at webkit.org>'s request for review:
Bug 82056: Use enumeration for CSS parser mode
https://bugs.webkit.org/show_bug.cgi?id=82056

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

------- Additional Comments from Nikolas Zimmermann <zimmermann at kde.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=133571&action=review


> Source/WebCore/ChangeLog:8
> +	   Introduce a new enumeration CSSParserMode to differ between strict,
quirks and svg presentation

Introduce a new CSSParserMode enum to differ between strict / quirks and SVG
presentation attribute parsing modes.

> Source/WebCore/ChangeLog:10
> +	   The next step will be updating StylePropertySet and CSSStyleSheet to
use this enumeration as well.

What does that mean?

> Source/WebCore/ChangeLog:12
> +	   After that it will be easier possible to reuse the CSS parser in SVG
as much as possible and
> +	   introduce SVG only functionality.

s/SVG only/SVG specific/

> Source/WebCore/css/CSSParser.cpp:189
> +inline bool CSSParser::inStrictMode()
> +{
> +    return m_cssParserMode == CSSStrictMode;
> +}

Why not move this in the header? And I'd make it const.

> Source/WebCore/css/CSSParser.cpp:191
> +inline bool CSSParser::inQuirksMode()

Ditto.

> Source/WebCore/css/CSSParser.cpp:193
> +    // FIXME: Move SVGAttributeMod to inStrictMode() once StylePropertySet
and CSSStyleSheet

typo: Mode

> Source/WebCore/css/CSSParser.cpp:198
>  CSSParser::CSSParser(bool strictParsing)

Hrm, why do you need a new constructor for this? I'd avoid this, even if its an
intermediate step.
CSSParser myParser(0) is now ambiguous.

> Source/WebCore/css/CSSParser.cpp:260
> +    ASSERT(cssParserMode & CSSStrictMode ^ cssParserMode & CSSQuirksMode);

This is pointless, now you moved away from the bitfield.

> Source/WebCore/css/CSSParser.cpp:1244
> +	   // Qirks mode and svg presentation attributes accept unit less
values.
> +	   if (!b && ((unitflags & (FLength | FAngle | FTime)) &&
(!value->fValue || cssParserMode == CSSQuirksMode || cssParserMode ==
SVGAttributeMode))) {

So why not introduce a "static inline bool
shouldAcceptUnitLessValues(CSSParserMode)" ?
This avoids the extra-comment and makes the code more readable.

> Source/WebCore/css/CSSParser.cpp:1599
> -		(id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText
&& !m_strict)) {
> +		(id >= CSSValueWebkitFocusRingColor && id < CSSValueWebkitText
&& inQuirksMode())) {

Shall we allow these color values for SVG as well? If not I'd make that
!inStrictMode().

> Source/WebCore/css/CSSParser.cpp:1646
> -	       } else if (!m_strict && value->id == CSSValueHand) // MSIE 5
compatibility :/
> +	       } else if (inQuirksMode() && value->id == CSSValueHand) // MSIE
5 compatibility :/

This for sure won't be needed for SVG, so this should be !inStructMode() no?

> Source/WebCore/css/CSSParser.cpp:1655
> -	   if (!m_strict && value->id == CSSValueHand) { // MSIE 5
compatibility :/
> +	   if (inQuirksMode() && value->id == CSSValueHand) { // MSIE 5
compatibility :/

Ditto.

> Source/WebCore/css/CSSParser.cpp:1765
> -	   validPrimitive = (!id && validUnit(value, FLength | FPercent,
m_strict));
> +	   validPrimitive = (!id && validUnit(value, FLength | FPercent,
m_cssParserMode));

I would have created a new private validUnit() function that does NOT accept a
CSSParserMode or bool argument, and just forwards to validUnit(param1, param1,
.. m_cssParserMode) to avoid having to pass it around everywhere.
If you'd land that first as separate patch the size of this patch would shrink
a lot!

> Source/WebCore/css/CSSParser.cpp:2236
> -	       validPrimitive = validUnit(value, FLength, true);
> +	       validPrimitive = validUnit(value, FLength, CSSStrictMode);

Ditto

> Source/WebCore/css/CSSParser.cpp:2245
> +	   validPrimitive = (!id && (value->unit ==
CSSPrimitiveValue::CSS_PERCENTAGE || value->fValue) && validUnit(value,
FInteger | FPercent | FNonNeg, CSSQuirksMode));

Ditto.

> Source/WebCore/css/CSSParser.cpp:2264
> +	   if (id == CSSValueAuto || validUnit(value, FInteger | FNonNeg,
CSSStrictMode))

Ditto.

> Source/WebCore/css/CSSParser.cpp:2269
> +	   if (id == CSSValueNoLimit || validUnit(value, FInteger | FNonNeg,
CSSStrictMode))

Ditto.

> Source/WebCore/css/CSSParser.cpp:2300
> -	       || (id >= CSSValueWebkitFocusRingColor && id <
CSSValueWebkitText && !m_strict)) {
> +	       || (id >= CSSValueWebkitFocusRingColor && id <
CSSValueWebkitText && inQuirksMode())) {

Should be irrelevant for SVG: so use !inStrictMode() here.

> Source/WebCore/css/CSSParser.cpp:3165
> +	   (id >= CSSValueGrey && id < CSSValueWebkitText && inQuirksMode()))

Ditto.