[webkit-reviews] review denied: [Bug 52627] Accelerated canvas2D has bad clipping performance : [Attachment 79283] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Stephen White
<senorblanco at chromium.org>'s request for review:
Bug 52627: Accelerated canvas2D has bad clipping performance
https://bugs.webkit.org/show_bug.cgi?id=52627

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79283&action=review

R- for a printf() left in, but looks very cool.  I think Ken or Chris should
sanity check the libtess usage as that part's all greek to me.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:57
> +const int pathTesselation = 30;

nit: static. What does this value (30) mean?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:74
> +	   , m_clippingPaths()

why doesn't copying a State object copy the clipping paths as well?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:86
> +static inline FloatPoint operator*(const FloatPoint& f, float scale)

It's kind of weird to define these here.  Why not in FloatPoint/FloatSize.h?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:106
> +class Quadratic {

should this go in its own file?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:127
> +class Cubic {

should this go in its own file?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:254
> +    Color red(255, 0, 0, 255);
> +    fillPath(path, red);

why red?

looking at beginStencilDraw() this doesn't seem to actually put red anywhere,
so maybe make this a more descriptively named constant would be clearer

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:261
> +    printf("clipOut\n");

do you mean to check this in?

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:285
> +	       for (pathIter = clippingPaths.begin(); pathIter <
clippingPaths.end();
> +		    ++pathIter) {

nit: odd wrapping

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:312
> +void GLES2Canvas::drawTexturedRect(Texture* texture, const FloatRect&
srcRect, const FloatRect& dstRect, const AffineTransform& transform, float
alpha, ColorSpace colorSpace, CompositeOperator compositeOp, bool clip)

enum preferred for the 'clip' parameter as drawTextureRect(..., Clip) /
drawTexturedRect(..., NoClip) or some better names are easier to grok at the
callsites.

> Source/WebCore/platform/graphics/chromium/GLES2Canvas.cpp:380
> +static void interpolateQuadratic(FloatVector* vertices, const SkPoint p0,
const SkPoint& p1, const SkPoint& p2)

these helpers also feel out of place in this file