[Webkit-unassigned] [Bug 38845] Canvas color property getters should serialize them according to spec

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38845


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #56075|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #16 from Darin Adler <darin at apple.com>  2010-05-14 09:20:26 PST ---
(From update of attachment 56075)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.