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

[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 55797: Proposed patch v4
https://bugs.webkit.org/attachment.cgi?id=55797&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
I see quite a few code changes here without corresponding tests that exercise
those functions. Are there already tests covering all these code paths? Which
tests?

> +	   * fast/dom/canvasContext2d-element-attribute-js-null-expected.txt:
Updated baseline.
> +	   * fast/dom/canvasContext2d-element-attribute-js-null.html:

We should move these tests into fast/canvas. Doesn't need to be part of this
patch, though.

> +PASS ctx.strokeStyle is '#000000'
> +PASS ctx.fillStyle is '#000000'
> +PASS ctx.shadowColor is 'rgba(0, 0, 0, 0.0)'
> +PASS ctx.strokeStyle is '#ff0000'
> +PASS ctx.strokeStyle is '#ffffff'
> +PASS ctx.strokeStyle is '#ffffff'
> +PASS ctx.fillStyle is '#ff0000'
> +PASS ctx.fillStyle is '#ffffff'
> +PASS ctx.fillStyle is '#ffffff'
> +PASS ctx.fillStyle is '#00ff00'
> +PASS ctx.shadowColor is '#ff0000'
> +PASS ctx.shadowColor is '#ffffff'
> +PASS ctx.shadowColor is '#ffffff'
> +PASS ctx.shadowColor is 'rgba(1, 2, 3, 0.40000)'

The way this test is written does not make very good use of the shouldBe
macros. It's always a danger sign when the test output shows only the results
and not what is being tested. It's easy to do better by doing this:

    shouldBe("ctx.strokeStyle = 'red'; ctx.strokeStyle", "'#ff0000'");

Or you can make helper functions. Either way, the test output is a lot clearer
when the expression includes the code being tested, not just something that
fetches the test result.

> +    state().m_shadowColor = Color(rgba);

There should be no need to write "Color(rgba)" here instead of just "rgba".

> -    state().m_shadowColor = color;
> +    state().m_shadowColor = Color(color);

Why do some call sites parse colors with CSSParser::parseColor, but this one
uses the Color constructor? Could you add test cases to show the different
behavior is correct?

> +    // Default to transparent black
> +    RGBA32 rgba = state().m_shadowColor.isValid() ?
state().m_shadowColor.rgb() : 0;
>      c->setShadow(IntSize(width, -height), state().m_shadowBlur,
Color(colorWithOverrideAlpha(rgba, alpha)), DeviceColorSpace);

Color's rgb() function already returns 0 for invalid colors, so there's no need
for the extra code here.

>      RGBA32 rgba = 0; // default is transparent black
> -    if (!state().m_shadowColor.isEmpty())
> -	   CSSParser::parseColor(rgba, state().m_shadowColor);
> +    if (state().m_shadowColor.isValid())
> +	   rgba = state().m_shadowColor.rgb();

Color already does this 0 default, so we don't need an if statement.

> -	       String m_shadowColor;
> +	       Color m_shadowColor;

The real reason to use Color instead of RGBA32 is that Color has a distinct
"invalid" value that can be distinguished from transparent black. If we don't
need that, then we may wnat to consider using RGBA32 here instead of Color to
save storage.

> +CanvasStyle::CanvasStyle(const Color& color)

We could consider taking an RGBA32 here instead for the same reasons mentioned
above.

> +    // We're only supporting 255 levels of gray here.  Since this isn't
> +    // even part of HTML5, I don't expect anyone will care.	If they do
> +    // we'll make a fancier Color abstraction.

I don't think this comment is helpful. It's correct, but doesn't seem to add
anything. For one thing, we also only support 255 levels of R, G, B, and A in
all the other constructors yet they have no comment.

On the other hand, changing the behavior of the existing code to always round
colors to 255 gray levels and 255 levels of R, G, B, and A will likely change
the behavior on Mac OS X, removing a feature from the canvas on that platform
that some applications may depend on. I wonder what the best way to find this
out is?

> +    case RGBA:
> +	   if (m_color.isValid())
> +	       context->setStrokeColor(m_color.rgb(), DeviceColorSpace);
> +	   break;

I don't understand the handling for m_color's invalid state. When would m_color
be invalid? Is there a test case that covers this code path?

> +bool CanvasStyle::isValid() const
> +{
> +    switch (m_type) {
> +    case RGBA:
> +	   return m_color.isValid();
> +    default:
> +	   return true;
> +    }
> +}

Is this really needed?

> +    // FIXME: What should we do with invalid colors?
> +    if (!color.isValid())
> +	   return String("rgba(0, 0, 0, 0.0)");

And why do we even have the concept of an invalid color in this class?

>  namespace WebCore {
>  
> +String serializedCanvasColor(const Color& color);
> +
>      class CanvasGradient;
>      class CanvasPattern;
>      class GraphicsContext;

The function should be indented as the rest of the namespace contents is, and
should be after the forward declarations, not before them.