[webkit-reviews] review denied: [Bug 9506] Implement HSL, and HSLA colors in WebKit : [Attachment 9155] patch, test cases and change logs

[bugzilla-request-daemon at opendarwin.org]

Darin Adler <darin at apple.com> has denied Darin Adler <darin at apple.com>'s
request for review:
Bug 9506: Implement HSL, and HSLA colors in WebKit
http://bugzilla.opendarwin.org/show_bug.cgi?id=9506

Attachment 9155: patch, test cases and change logs
http://bugzilla.opendarwin.org/attachment.cgi?id=9155&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
Looks pretty good. I have some relatively unimportant comments, but I think I'm
going to review- so you can address at least some of them.

Some minor formatting mistakes:

+    ValueList *args = value->function->args;
+    Value *v = args->current();

Please put the * next to tyhe type.

+    colorValues[0] = ((((int)(v->fValue) % 360) + 360) % 360)/360.;

Need spaces around all the operators, including the "/" and we prefer 360.0 to
360. for double constants.

+    for (int i = 1; i < 3; i++)
+    {

Please put the { on the same line as the for statement.

+    colorValues[0] = ((((int)(v->fValue) % 360) + 360) % 360)/360.;

What does the line above to if fValue does not fit in an int? Need to check if
it's in range?

+	 colorValues[i] = v->fValue/100.; // needs to be value between 0 and
1.0

This code says it "needs to be" -- what checks that the number is between 0 and
100?

-	 if (!validUnit(v, FInteger|FPercent, true))

Spaces around operators like "|" too.

In makeRGBAfromHSLA, we'd normally capitalize the f.

+	 bool parseColorParameters(Value* value, int* colorValues, bool
parseAlpha);
+	 bool parseHSLParameters(Value* value, float* colorValues, bool
parseAlpha);

We'd normally leave out the word "value" in these declarations.

+float calcHue(float temp1, float temp2, float hueVal)

Is there a better name than temp1/temp2 for these? If not, can you refer to
place you found the algorithm?

+RGBA32 makeRGBAfromHSLA(float h, float s, float l, float a)

Why the use of float here, given the use of double in the code that's passing
things in and the use of double-precision constants like "1.0"? This code is
going to do some conversion from float to double and back that we might be able
to avoid.

The HSV code in this file references the URL of the place the color algorithm
came from. Can you do the same with this new code?