Closed Bug 288796 Opened 19 years ago Closed 19 years ago

Add public ParseColorString method


(Core :: CSS Parsing and Computation, defect)

Not set





(Reporter: vlad, Assigned: vlad)




(1 file, 3 obsolete files)

Patch adds a ParseColorString method to nsICSSParser, for use by things (such as
<canvas>) that need to parse CSS colors outside of a stylesheet.  It also adds a
HandleAlphaColors flag which can be set to allow the parser to understand
rgba()/hsla(), default to false.
My fear of recycled parsers suggests I'd prefer not having SetHandleAlphaColors,
but instead having the boolean be a parameter to ParseColor, which would set the
member to the parameter at the start of the function and to PR_FALSE at the end.

I'll look more closely tomorrow...
Either way works for me; I assumed you meant have a setter method like the
current flags.  Would that parameter have a C++ default of PR_FALSE, to avoid
having to change current callers?  (And to indicate common usage?)

Let me know if you'd like me to make the change.
Not a parameter to anything that exists now -- just a parameter to the
nsICSSParser method, to avoid having a setter on nsICSSParser and to avoid
accidentally leaving the parser in the state.  (i.e., modeled on mHTMLMediaMode)
Attached patch cssparser-color-2.patch (obsolete) — Splinter Review
Try this one on for size.
Assignee: dbaron → vladimir
Attachment #179421 - Attachment is obsolete: true
Comment on attachment 179931 [details] [diff] [review]

>+                              nscolor& aColor,
>+                              PRBool aHandleAlphaColors);

Switch the order of these two so the out param is last, and use nscolor*
instead of nscolor& .  (This is basically an XPCOM interface.)

>Index: nsCSSParser.cpp

Could you change SetParsingCompoundProperty, SetCaseSensitive, and SetSVGMode
so they assert that their argument is either PR_TRUE or PR_FALSE (since PRBool
can hold other values, unfortunately).

>+  mHandleAlphaColors = aHandleAlphaColors;
>+  rv = InitScanner(aBuffer, aURL, aLineNumber, aURL);
>+  if (NS_FAILED(rv))
>+    return rv;

Switch the order of these two things (so mHandleAlphaColors isn't touched until
you know you will undo it).

>+  nsCSSValue value;
>+  if (ParseColor(rv, value)) {
>+    if (value.GetUnit() == eCSSUnit_String) {
>+      nsAutoString s;
>+      nscolor rgba;
>+      if (NS_ColorNameToRGB(value.GetStringValue(s), &rgba)) {

It looks like you can assert that this succeeded rather than checking it, based
on ParseColor.

>+        aColor = rgba;
>+        rv = NS_OK;
>+      } else {
>+        rv = NS_ERROR_FAILURE;
>+      }

>+    } else if (value.GetUnit() == eCSSUnit_Color) {
>+      aColor = value.GetColorValue();
>+      rv = NS_OK;
>+    } else if (value.GetUnit() == eCSSUnit_Integer) {
>+      rv = NS_ERROR_FAILURE;

Are you really supposed to be ignoring system colors and currentColor?	If so,
that should be documented in the header file (nsICSSParser.h), and have a
comment here explaining that that's what this is doing.
Attached patch cssparser-color-3.patch (obsolete) — Splinter Review
Changes made
Attachment #179931 - Attachment is obsolete: true
Attachment #180503 - Flags: review?(dbaron)
Fixed missing = 0; in nsICSSParser interface
Comment on attachment 180503 [details] [diff] [review]

This still doesn't handle currentColor in the integer case -- you need an
additional parameter to the parser to handle that.  I'm not crazy about the
nsILookAndFeel dependency, but I guess it's OK.
Is it ok to just add in a comment and return failure for currentColor?  Or
better yet, specify that in the currentColor case, that the nscolor param won't
be modified?
Added comment in nsICSSParser.h describing handling of colors that aren't fully
specified by the string (excluding colors that can be obtained from LaF), and
handle the <0 case for Integer values.	Also cleaned up unnecessary setting of
rv to error conditions.
Attachment #180503 - Attachment is obsolete: true
Attachment #180994 - Flags: review?(dbaron)
Comment on attachment 180994 [details] [diff] [review]

>+      // which we don't handle as per the ParseColorString definition
>+      rv = NS_ERROR_FAILURE;

This is probably something we want to fix, so add "XXX" at the start of the
comment, and r=dbaron.
Attachment #180994 - Flags: review?(dbaron) → review+
XXX added; r+sr=dbaron or should I poke around for a quick sr?
Attachment #180994 - Flags: superreview+
Comment on attachment 180994 [details] [diff] [review]

Attachment #180994 - Flags: approval1.8b2+
Landed, thanks!
Closed: 19 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.