[webkit-reviews] review denied: [Bug 82560] Implement canvas v5 line dash feature : [Attachment 162762] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Justin Novosad <junov at google.com>'s
request for review:
Bug 82560: Implement canvas v5 line dash feature
https://bugs.webkit.org/show_bug.cgi?id=82560

Attachment 162762: Patch
https://bugs.webkit.org/attachment.cgi?id=162762&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=162762&action=review


This patch seems great and almost ready to go. But I am not comfortable with
the mix of float and double in our canvas element implementation without any
rationale or plan for moving forward. I see how it’s handy to use doubles for
this new feature, but we need to look at this in context, and not in isolation.


> Source/WebCore/bindings/js/JSDOMBinding.h:394
> +	       if (exec->hadException())
> +		   return false;
> +
> +	       return true;

I think this reads better as:

    return !exec->hadException();

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:516
> +static bool validateLineDash(const Vector<double>& dash)

I would name this something more like:

    lineDashSequenceIsValid

Using a verb phrase to name a function that returns a boolean with no side
effects raises the question of whether there are side effects, and also makes
it unclear what the boolean result of the function means.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:533
> +    if (dash.size() % 2)
> +	   modifiableState().m_lineDash.append(dash);

This mysterious code needs a brief "why" comment. You shouldn’t assume that the
person reading the code has read the specification and so understands the need
for this peculiar rule.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:581
> +    DashArray convertedLineDash;
> +    convertedLineDash.resize(state().m_lineDash.size());

The correct way to allocate a vector with an initial size is to pass it into
the constructor. If you make a separate function call it’s unnecessarily less
efficient.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.h:257
> +	   Vector<double> m_lineDash;
> +	   double m_lineDashOffset;

This is not good. After this patch the canvas implementation now has a mix of
double and float and there is no rationale for which items are float and which
are double. Until this patch it was all float.

> Source/WebCore/html/canvas/CanvasRenderingContext2D.idl:-77
> -	   // FIXME: These attributes should also be implemented for V8.

Why did you remove this FIXME? Is the legacy WebKit line dash feature something
we’ll never do in V8-based builds of WebKit? If so, maybe a "why" comment is in
order.

> Source/WebCore/platform/graphics/DashArray.h:32
> +typedef CGFloat DashArrayEntry;

I think perhaps the word Element rather than Entry would be better?