[webkit-reviews] review denied: [Bug 38845] Canvas color property getters should serialize them according to spec : [Attachment 56075] Proposed patch v5

[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 38845: Canvas color property getters should serialize them according to
spec
https://bugs.webkit.org/show_bug.cgi?id=38845

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

------- Additional Comments from Darin Adler <darin at apple.com>
This is looking good. Still needs some work, though.

> -    , m_shadowColor("black")
> +    , m_shadowColor(Color::transparent)

Is this change correct? Does it behave like this in the other browsers? What
does HTML5 say about it?

> @@ -814,15 +815,18 @@ void CanvasRenderingContext2D::setShadow(float width,
float height, float blur)
>  {
>      state().m_shadowOffset = FloatSize(width, height);
>      state().m_shadowBlur = blur;
> -    state().m_shadowColor = "";
> +    state().m_shadowColor = Color::transparent;
>      applyShadow();
>  }

Where's the test case for this? How does it behave in the other browsers? What
does HTML5 say about it? I have the same question about many of the setShadow
overloads and about clearShadow. I don't see tests that use them and then check
the value of the shadowColor property, yet that's the code we're changing.

> +    , m_valid(true)

For boolean members, the WebKit style has been to complete the phrase "style
<xxx>". So this would be m_isValid rather than m_valid.

> +    if ((m_valid = CSSParser::parseColor(m_rgba, color)))
> +	   m_rgba = colorWithOverrideAlpha(m_rgba, alpha);

I think this code would be slightly clearer if it used a local variable for the
result of parseColor instead of setting m_rgba first to the wrong value and
then the right on.

I don't think we need to complicate things by adding in the notion of an
invalid canvas style.

Instead the color parsing from the CanvasStyle constructors that take color
strings should move into the CanvasStyle::create functions. Those functions
will return 0 if the color cannot be parsed and create a CanvasStyle if the
color can be parsed. The CanvasStyle constructors that take color strings would
be removed.

> +	   CMYKAValues* m_cmyka;

I suggest using an OwnPtr for this instead of writing the delete explicitly.
But I also don't see the need to heap allocate this.

There are a lot of changes here without test cases -- I'd like to see either
new or existing test cases that cover all the modified functions.

review- mainly because we need better test coverage