[webkit-reviews] review denied: [Bug 46112] Colors seem to be parsed using HTML quirks in SVG attributes : [Attachment 71695] Patch

[bugzilla-daemon at webkit.org]

Dirk Schulze <krit at webkit.org> has denied Rob Buis <rwlbuis at gmail.com>'s
request for review:
Bug 46112: Colors seem to be parsed using HTML quirks in SVG attributes
https://bugs.webkit.org/show_bug.cgi?id=46112

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

------- Additional Comments from Dirk Schulze <krit at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71695&action=review

Do we need pixel tests for this patch? If so, you should have green rects as
result images.

> WebCore/css/CSSMutableStyleDeclaration.h:136
> +    void setStrictParsing(bool b) { m_parseMode = b ? HTMLStrictMode :
HTMLQuirksMode; }

this should take an enum. We talked about it on IRC. You fear that the patch
will be to big if you change it in a row, right? But you should mention it in
the ChangeLog, add a FIXME that points to the bug and even better, have a
followup ready.

> WebCore/css/CSSParser.h:218
> +	   inline bool strict() const { return m_parseMode == HTMLStrictMode; }


We have 2 strict modes, HTMLStrictMode and SVGStrictMode, the function name
should be self-explanatory. Also, what does HTMLStrictMode mean for
SVGStrictMode? Does HTMLStrictMode include SVGStrictMode? If yes we should
rethink the enum entries. Or are they exclusive?