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.
Needs to be done, but low prio.
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.
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?
Another question, is this line + if (s.startsWith("hsl") || s.startsWith("rgba") || supposed to be s.startsWith("rgba") and not s.startsWith("rgb") ?
Comment on attachment 16022 [details] First attempt r- until the above questions are answered.
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.
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.
Rob, Sounds good. If you would add a comment in the code about those 2 cases it would be great.
Created attachment 16035 [details] Added some comments Added some comments as per Sam's suggestion. Cheers, Rob.
Comment on attachment 16035 [details] Added some comments Talked to Rob in IRC about expanding the explanation in the comments to be more.
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.
Landed in r25183.