[webkit-reviews] review granted: [Bug 23065] Enable incremental painting for canvas : [Attachment 26350] Patch, testcase, changelog.

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 23065: Enable incremental painting for canvas
https://bugs.webkit.org/show_bug.cgi?id=23065

Attachment 26350: Patch, testcase, changelog.
https://bugs.webkit.org/attachment.cgi?id=26350&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> -const char* defaultFont = "10px sans-serif";
> +static const char* defaultFont = "10px sans-serif";

This is still not doing what the original author probably intended. This is a
variable, not a constant. It should be:

    static const char defaultFont[] = 10px sans-serif";

or

    static const char* const defaultFont = 10px sans-serif";

> +class CanvasStrokeStyleApplier : public StrokeStyleApplier {
> +public:
> +    CanvasStrokeStyleApplier(CanvasRenderingContext2D* canvasContext)
> +	   : m_canvasContext(canvasContext)
> +    {
> +    }
> +    
> +    virtual void strokeStyle(GraphicsContext* c)
> +    {
> +	   c->setStrokeThickness(m_canvasContext->lineWidth());
> +
> +	   LineCap cap;
> +	   if (parseLineCap(m_canvasContext->lineCap(), cap))
> +	       c->setLineCap(cap);
> +
> +	   LineJoin join;
> +	   if (parseLineJoin(m_canvasContext->lineJoin(), join))
> +	       c->setLineJoin(join);
> +
> +	   c->setMiterLimit(m_canvasContext->miterLimit());
> +    }
> +
> +    CanvasRenderingContext2D* m_canvasContext;
> +};

Both strokeStyle and m_canvasContext should be private, not public. Because ...
because why not? I say always make things private unless they're needed
publicly.

> -void CanvasRenderingContext2D::willDraw(const FloatRect& r)
> +void CanvasRenderingContext2D::willDraw(const FloatRect& r, bool
applyTransform, bool applyShadow, bool applyClip)

Functions that take a bunch of booleans make the code hard to read. This should
either use an enum for each boolean or a flags word. Either is easier to read
at the call site because of the use of named constants.

> +    if (fill)
> +	   m_canvas->willDraw(textRect);
> +    else
> +	   // When stroking text, pointy miters can extend outside of textRect,
so we
> +	   // punt and dirty the whole canvas.
> +	   m_canvas->willDraw(FloatRect(0, 0, m_canvas->width(),
m_canvas->height()));

Need braces around multi-line else body here (number of lines includes things
like comments).

Would be better to have a helper function for "will draw the entire canvas".
Code that constructs the rectangle is confusing.

>      CanvasStyle* drawStyle = fill ? state().m_fillStyle.get() :
state().m_strokeStyle.get();
>      if (drawStyle->canvasGradient() || drawStyle->canvasPattern()) {
> +	   // FIXME The rect is not big enough for miters on stroked text

We should have a colon after the FIXME. We should have a period at the end of
the sentence.

I don't understand why it's OK to check in with this FIXME. Will this result in
incorrect canvas drawing?

> +// Map rect r from srcRect to an equivalent rect in destRect

Sentence-capitalized comment needs a period.

> +inline FloatRect mapRect(const FloatRect& r, const FloatRect& srcRect, const
FloatRect& destRect)
> +{
> +    float widthScale = destRect.width() / srcRect.width();
> +    float heightScale = destRect.height() / srcRect.height();
> +    return FloatRect(destRect.x() + (r.x() - srcRect.x()) * widthScale,
> +			destRect.y() + (r.y() - srcRect.y()) * heightScale,
> +			r.width() * widthScale, r.height() * heightScale);
> +}

Why is this inline? Is the behavior with 0 width and 0 height acceptable?

Does this really work reliably? Our previous attempts to turn this on have had
bad results. Is the test you added extensive enough to prove we won't have
trouble? Did you test with the sites that exercise canvas heavily, like the one
that does the 3D Doom-style game?

I'm going to say r=em, because none of these comments seem like showstoppers to
me