Bug 12059 – SVG colors have two separate parsing paths (one CSS and one SVGColor::setRGBColor)

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Eric Seidel (no email)]

SVG colors have two separate parsing paths (one CSS and one SVGColor::setRGBColor)

That's just an accident waiting to happen.  Ideally these should be unified into a single parser.

[Rob Buis]

Needs to be done, but low prio.

[Rob Buis]

Created attachment 16022 [details]
First attempt

This tries to combine the two codepaths. Note that unfortunately I have to test for rgba,hsl and hsla constructs. One way to avoid it may be to make the parser aware that it is parsing svg css, but that
needs more discussion I suppose. One further thing that could fix is to parse SVG color property and
deliver a SVGColor, not a CSSPrimitiveValue like now.
Cheers,

Rob.

[Sam Weinig]

in RGBA32 CSSParser::parseColor(const String& string, bool strict)

+    parseColor(color, string, strict);
+    return color;

Is it okay to ignore the return boolean of parseColor here? 

[Sam Weinig]

Another question, is this line

+    if (s.startsWith("hsl") || s.startsWith("rgba") ||

supposed to be s.startsWith("rgba") and not  s.startsWith("rgb") ?

[Sam Weinig]

Comment on attachment 16022 [details]
First attempt

r- until the above questions are answered.

[Rob Buis]

Hi,

(In reply to comment #4)
> Another question, is this line
> 
> +    if (s.startsWith("hsl") || s.startsWith("rgba") ||
> 
> supposed to be s.startsWith("rgba") and not  s.startsWith("rgb") ?

Yes, if you look at CSSParser::parseColorFromValue you see rgb( is accepted by svg, but a check is made for rgba(, hsl( and hsla( to not be accepted if the svg flag is true.
Cheers,

Rob.

[Rob Buis]

Hi Sam,

(In reply to comment #3)
> in RGBA32 CSSParser::parseColor(const String& string, bool strict)
> 
> +    parseColor(color, string, strict);
> +    return color;
> 
> Is it okay to ignore the return boolean of parseColor here? 

Well, the old behaviour returned transparent color (all 0) but still valid color unless a color was found, I am emulating that behaviour here.
Cheers,

Rob.

[Sam Weinig]

Rob,

Sounds good.  If you would add a comment in the code about those 2 cases it would be great.

[Rob Buis]

Created attachment 16035 [details]
Added some comments

Added some comments as per Sam's suggestion.
Cheers,

Rob.

[Sam Weinig]

Comment on attachment 16035 [details]
Added some comments

Talked to Rob in IRC about expanding the explanation in the comments to be more.

[Rob Buis]

Created attachment 16062 [details]
Slightly different approach

This one has just one method and enables the client to set up a rgba32 default value, like what has happened in the past for html canvas code.
Cheers,

Rob.

[Rob Buis]

Landed in r25183.