[webkit-reviews] review denied: [Bug 82335] Introduce CSSParserMode in all classes : [Attachment 134353] Patch

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has denied Dirk Schulze <krit at webkit.org>'s
request for review:
Bug 82335: Introduce CSSParserMode in all classes
https://bugs.webkit.org/show_bug.cgi?id=82335

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

------- Additional Comments from Ryosuke Niwa <rniwa at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=134353&action=review


> Source/WebCore/css/CSSImportRule.cpp:72
> +    bool strict = cssParserMode == CSSStrictMode;

It seems like this boolean is redundant. You can move this condition to
enforceMIMEType.

> Source/WebCore/css/CSSParser.cpp:329
> +static inline bool isStrictParserMode(CSSParserMode cssParserMode)
> +{
> +    // FIXME: SVG presnetation attribute values should be parsed in strict
mode as well.
> +    return cssParserMode == CSSStrictMode;
> +}

It seems like this function should be next to where CSSParserMode is defined.

> Source/WebCore/css/CSSParserMode.h:34
> -enum CSSParserMode {
> -    CSSQuirksMode,
> +enum {
> +    CSSQuirksMode = 1,

This doesn't look right. Why does CSSQuirksMode need to be 1?
Also why do we need to convert CSSParserMode to an unsigned char?
That should be a trick only used wherever bit fields are defined.
If we use unsigned char everywhere, then compiler won't warn us when we add new
values
and forgot to add it to a switch statement.
r- because of this.

> Source/WebCore/css/CSSParserMode.h:49
> +inline CSSParserMode inCSSParserMode(CSSParserMode cssParserMode)
> +{
> +    return static_cast<CSSParserMode>(cssParserMode);
> +}

This function should be unnecessary.