[webkit-reviews] review denied: [Bug 40273] The HTML5 canvas 2d.fillStyle.parse.current.* tests do not pass : [Attachment 59827] Proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Andreas Kling
<andreas.kling at nokia.com>'s request for review:
Bug 40273: The HTML5 canvas 2d.fillStyle.parse.current.* tests do not pass
https://bugs.webkit.org/show_bug.cgi?id=40273

Attachment 59827: Proposed patch
https://bugs.webkit.org/attachment.cgi?id=59827&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
The bug title should be "add support for special color named currentColor".
This patch is about adding a missing feature, more than about fixing a
particular test case in a test suite!

> +    RGBA32 rgba = Color::black;
> +    if (canvas->inDocument())
> +	   CSSParser::parseColor(rgba,
canvas->style()->getPropertyValue(CSSPropertyColor));
> +    return rgba;

WebKit normally uses early return style. So it would be like this:

    if (!canvas->inDocument())
	return Color::black;
    RGBA32 color = Color::black;
    CSSParser::parseColor(color,
canvas->style()->getPropertyValue(CSSPropertyColor));
    return rgba;

I don't think this function needs "from canvas" in its name. I think
currentColor would be fine.

> +    if (color.lower() == "currentcolor")

The most efficient way to do this check is:

    equalIgnoringCase(color, "currentcolor")

Among other things, calling lower() means we'll allocate a new string if there
are any uppercase characters.

I'm not that fond of a function named "useCurrentColor". I think you would want
to name it something more like parseColorOrCurrentColor. Another possibility
would be to name it parseColor and make it a private member of
CanvasRenderingContext2D. I think I like that design best.

> +    if (!useCurrentColor(canvas(), state().m_shadowColor, color)) {
> +	   if (!CSSParser::parseColor(state().m_shadowColor, color))
> +	       return;
> +    }

This code calls parseColor twice. Once inside useCurrentColor and once here at
the call site. I suggest the parseColorOrCurrentColor design and removing the
extra call to parseColor.

> +    if (color.lower() == "currentcolor")
> +	   return adoptRef(new CanvasStyle());

We should not have the string "currentcolor" in two different places in the
code. We should find a way to share this. A single that would have a parseColor
function that can return either failure, flag indicating current color, or an
RGBA value. Then CanvasRenderingContext2D and CanvasStyle can both use it. I
think it would be good to put it in the CanvasStyle header. One design would
be:

    enum ColorParseResult { ParsedRGBA, ParsedCurrentColor, ParseFailed };
    ColorParseResult parseColor(const String& colorString, RGBA& parsedColor);

This could be a free function in CanvasStyle.h, or a static function member of
CanvasStyle.

review- because at the very least we want to use equalIgnoringCase for this.

Do you think the test suite covers all the code paths here? Do we need to add
any new tests?