119807 - Computed color values should support getRGBColorValue

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

We need an nsIDOMRGBColor implementation (similar to nsIDOMRect).  Then we need
to do the right thing for colors instead of setting them to strings.

Comment 1

[Boris Zbarsky [:bzbarsky]]

No time to work on this.

Comment 2

[Christopher Aillon (sabbatical, not receiving bugmail)]

->Me

Comment 3

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed patch v1.0

Implement this baby!

Thanks go to Peterv for the mac project changes :)

Also breakup nsComputedDOMStyle to have a header file, and add picas to our
convertible float units.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Looks pretty good, all told.  A few comments:

1) Could you leave the static const nsCSSProperty queryableProperties in the
   .cpp file?
2) Could you leave as many headers as possible in the .cpp file?
3) Could you add comments in nsIDOMClassInfo.h and nsDOMClassInfo.cpp right
   before the lines you add saying what that stuff is?  Yes, I know CSSRect does
   not do it....  adding comments to that would be good too.  :)
4)  +        NS_NAMED_LITERAL_STRING(comma, ",");
   Shouldn't that have a space after the comma?
5) +    nsDOMCSSRGBColor *rgbColor = new nsDOMCSSRGBColor(red, green, blue);
+
+    red->SetNumber(NS_GET_R(aColor));
+    green->SetNumber(NS_GET_G(aColor));
+    blue->SetNumber(NS_GET_B(aColor));
+
+    return rgbColor;

This leaks red/green/blue if the |new nsDOMCSSRGBColor| fails.  And we _may_
want to throw out-of-memory exceptions when those |new| calls fail (further
upstream)

Comment 5

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 74620 [details] [diff] [review]
Proposed patch v1.0

Bah.  It's just as well that my first patch sucked so badly.

Looking at it again made me realize that |foo { color: ActiveBorder; }| (and
other system colors) had not been accounted for.

New and highly improved patch coming up.

Comment 6

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed fix v1.1

Okay, let's try this again :)

Comment 7

[Boris Zbarsky [:bzbarsky]]

>        .\$(OBJDIR)\nsDOMCSSRect.obj            \
>+       .\$(OBJDIR)\nsDOMCSSRGBColor.obj                \

Fix the spacing, please.

>- *
>- * This file is very much work in progress, there's a lot of code in this
>- * file that is #ifdef'd out placeholders.

Leave this in please.  It's still true till we get CSS2Properties hooked up...

>+      val->SetString(NS_LITERAL_STRING(""));
>+      return NS_ERROR_OUT_OF_MEMORY;

This leaks |val|.  You have this in a few places.  It also returns an
uninitialized pointer from a getter, no?  (even if you _are_ throwing an
exception)

>+    case CSS_IDENT :
>+      {
>+        tmpStr.Append(mValue.mString);

Isn't this identical to CSS_STRING so you can just make them share the code?

> +  void SetIdentifier(const nsACString& aString)

I'd be happier if this were called SetIdent, I think

Comment 8

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Proposed fix v1.2

>>	  .\$(OBJDIR)\nsDOMCSSRect.obj		  \
>>+	  .\$(OBJDIR)\nsDOMCSSRGBColor.obj		  \
>Fix the spacing, please.

Done.  Though the spacing is kind of screwy in general for me in viewing that
file because of the tabs (I have my editor set to display tabs to 4 spaces and
it doesn't look aligned at all in my editor).

>>- *
>>- * This file is very much work in progress, there's a lot of code in this
>>- * file that is #ifdef'd out placeholders.
>Leave this in please.	It's still true till we get CSS2Properties hooked up...


Ah, well I didn't remove it because I didn't it was true anymore.  Rather I
removed it because I thought it was obvious that it was a work in progress and
didn't need to be explicitly said.  Makes no difference either, so I put it
back in (with a grammar fix).

>>+	 val->SetString(NS_LITERAL_STRING(""));
>>+	 return NS_ERROR_OUT_OF_MEMORY;
>This leaks |val|.  You have this in a few places.

Oops.  Fixed.

>It also returns an uninitialized pointer from a getter, no?

Per conversation with bz (through email) we'll leave that alone for now since
we also do the same for the NS_ENSURE_TRUE call.

>>+    case CSS_IDENT :
>>+	 {
>>+	   tmpStr.Append(mValue.mString);
>Isn't this identical to CSS_STRING so you can just make them share the code?

Good catch.  Done.

>> +  void SetIdentifier(const nsACString& aString)
>I'd be happier if this were called SetIdent, I think

Heh, I actually had it called SetIdent until a few minutes before I diffed
attachment 74471 [details] [diff] [review].  I changed my mind at the last minute for some reason and
that's what got uploaded.  Changed it back.

Comment 9

[Boris Zbarsky [:bzbarsky]]

+    mValue.mColor = aColor;
+    NS_ADDREF(mValue.mRect);

Eeep.  Please addref the right thing.  :)

With that, r=bzbarsky

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 75026 [details] [diff] [review]
Proposed fix v1.2

- In nsComputedDOMStyle::GetDOMCSSRGBColor():

+  nsROCSSPrimitiveValue *red	= nsnull;
+  nsROCSSPrimitiveValue *green = nsnull;
+  nsROCSSPrimitiveValue *blue	= nsnull;

-   PRUint32 len = PR_snprintf(buf, 32, "#%02x%02x%02x",
-			       NS_GET_R(aColor),
-			       NS_GET_G(aColor),
-			       NS_GET_B(aColor));
+  red	 = GetROCSSPrimitiveValue();
+  green = GetROCSSPrimitiveValue();
+  blue  = GetROCSSPrimitiveValue();

Why not assign GetROCSSPrimitiveValue() directly into these variables when you
declare them in stead of the doing two assignments? Assigning null into these
variables here is pointless.

- In nsComputedDOMStyle::GetColor():

+    if (rgb)
+      val->SetColor(rgb);
+    else {

Please add braces around the if statement even if it's only a one line
statement. I found more of these in other places, fix all you can find.

- Eek:

-	   val->SetString(width); break;
+	   val->SetIdent(width); break;

break really needs to be on its own line! I know you didn't write this, but
please change it.

- nsROCSSPrimitiveValue::SetIdent() and ::Reset(). SetIdent() will set
mValue.mString to nuill in an OOM situation, then reset will crash since you're
passing null to nsMemory::Free(). Ideally SetIdent() and all other similar
methods would deal with OOM and let the caller know about the error, and then
make the state of the object such that it's safe to call Reset() on it later
on. Or alternatively Reset() should be safe against OOM.

- In nsIDOMClassInfo.h, don't change the comment style of the comment in the
enum from a C++ style comment to a C style comment. All other comments in that
same enum are C++ style comments, so why should this be different?

sr=jst if you fix that.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

Attachment: Fix v1.3

Includes bz's and jst's comments.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 75462 [details] [diff] [review]
Fix v1.3

looks good

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 75462 [details] [diff] [review]
Fix v1.3

looks good

Comment 14

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 75462 [details] [diff] [review]
Fix v1.3

transferring sr=jst

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 75462 [details] [diff] [review]
Fix v1.3

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 16

[Christopher Aillon (sabbatical, not receiving bugmail)]

Fix checked in.