Bug 12059 - SVG colors have two separate parsing paths (one CSS and one SVGColor::setRGBColor)
Summary: SVG colors have two separate parsing paths (one CSS and one SVGColor::setRGBC...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P3 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-01 11:53 PST by Eric Seidel (no email)
Modified: 2007-08-22 02:03 PDT (History)
1 user (show)

See Also:


Attachments
First attempt (12.39 KB, patch)
2007-08-19 10:08 PDT, Rob Buis
sam: review-
Details | Formatted Diff | Diff
Added some comments (12.54 KB, patch)
2007-08-20 11:50 PDT, Rob Buis
sam: review-
Details | Formatted Diff | Diff
Slightly different approach (17.38 KB, patch)
2007-08-21 13:29 PDT, Rob Buis
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2007-01-01 11:53:21 PST
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.
Comment 1 Rob Buis 2007-06-12 10:31:04 PDT
Needs to be done, but low prio.
Comment 2 Rob Buis 2007-08-19 10:08:09 PDT
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.
Comment 3 Sam Weinig 2007-08-19 17:49:31 PDT
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? 
Comment 4 Sam Weinig 2007-08-19 17:51:15 PDT
Another question, is this line

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

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

Comment 5 Sam Weinig 2007-08-19 17:54:01 PDT
Comment on attachment 16022 [details]
First attempt

r- until the above questions are answered.
Comment 6 Rob Buis 2007-08-19 23:48:56 PDT
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.
Comment 7 Rob Buis 2007-08-19 23:51:51 PDT
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.
Comment 8 Sam Weinig 2007-08-20 09:04:51 PDT
Rob,

Sounds good.  If you would add a comment in the code about those 2 cases it would be great.
Comment 9 Rob Buis 2007-08-20 11:50:24 PDT
Created attachment 16035 [details]
Added some comments

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

Rob.
Comment 10 Sam Weinig 2007-08-21 11:59:48 PDT
Comment on attachment 16035 [details]
Added some comments

Talked to Rob in IRC about expanding the explanation in the comments to be more.
Comment 11 Rob Buis 2007-08-21 13:29:33 PDT
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.
Comment 12 Rob Buis 2007-08-22 02:03:06 PDT
Landed in r25183.